From 5addb7a7e1190564bdbbb7d8e7c6acd828c3d66e Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 1/6] Revert "Add system.iceberg_tables table function" This reverts commit 5ce80becb339a75c05e332ca5aa47cd63672a7ce. --- .../trino/plugin/iceberg/IcebergModule.java | 5 +- .../plugin/iceberg/IcebergSplitManager.java | 4 - .../functions/IcebergFunctionProvider.java | 30 --- .../tables/IcebergTablesFunction.java | 129 ------------- .../tables/IcebergTablesFunctionProvider.java | 42 ---- .../BaseIcebergConnectorSmokeTest.java | 52 ----- .../BaseIcebergMinioConnectorSmokeTest.java | 30 --- .../iceberg/BaseSharedMetastoreTest.java | 7 - .../TestSharedHiveThriftMetastore.java | 181 ------------------ ...ergUnityRestCatalogConnectorSmokeTest.java | 12 +- 10 files changed, 3 insertions(+), 489 deletions(-) delete mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java delete mode 100644 plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunctionProvider.java delete mode 100644 plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSharedHiveThriftMetastore.java diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java index 386f7dd221b9..0992e51d2375 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java @@ -42,7 +42,6 @@ import io.trino.plugin.iceberg.functions.IcebergFunctionProvider; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesFunctionProcessorProviderFactory; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesFunctionProvider; -import io.trino.plugin.iceberg.functions.tables.IcebergTablesFunctionProvider; import io.trino.plugin.iceberg.procedure.AddFilesTableFromTableProcedure; import io.trino.plugin.iceberg.procedure.AddFilesTableProcedure; import io.trino.plugin.iceberg.procedure.DropExtendedStatsTableProcedure; @@ -136,9 +135,7 @@ public void configure(Binder binder) tableProcedures.addBinding().toProvider(AddFilesTableProcedure.class).in(Scopes.SINGLETON); tableProcedures.addBinding().toProvider(AddFilesTableFromTableProcedure.class).in(Scopes.SINGLETON); - Multibinder tableFunctions = newSetBinder(binder, ConnectorTableFunction.class); - tableFunctions.addBinding().toProvider(TableChangesFunctionProvider.class).in(Scopes.SINGLETON); - tableFunctions.addBinding().toProvider(IcebergTablesFunctionProvider.class).in(Scopes.SINGLETON); + newSetBinder(binder, ConnectorTableFunction.class).addBinding().toProvider(TableChangesFunctionProvider.class).in(Scopes.SINGLETON); binder.bind(FunctionProvider.class).to(IcebergFunctionProvider.class).in(Scopes.SINGLETON); binder.bind(TableChangesFunctionProcessorProviderFactory.class).in(Scopes.SINGLETON); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitManager.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitManager.java index 0cb4d8d849f3..f23983a4a2c3 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitManager.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitManager.java @@ -21,7 +21,6 @@ import io.trino.plugin.base.classloader.ClassLoaderSafeConnectorSplitSource; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesFunctionHandle; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesSplitSource; -import io.trino.plugin.iceberg.functions.tables.IcebergTablesFunction.IcebergTables; import io.trino.spi.connector.ConnectorSession; import io.trino.spi.connector.ConnectorSplitManager; import io.trino.spi.connector.ConnectorSplitSource; @@ -158,9 +157,6 @@ public ConnectorSplitSource getSplits( .toSnapshot(functionHandle.endSnapshotId())); return new ClassLoaderSafeConnectorSplitSource(tableChangesSplitSource, IcebergSplitManager.class.getClassLoader()); } - if (function instanceof IcebergTables icebergTables) { - return new ClassLoaderSafeConnectorSplitSource(new FixedSplitSource(icebergTables), IcebergSplitManager.class.getClassLoader()); - } throw new IllegalStateException("Unknown table function: " + function); } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java index b23b543a3cbf..07326a42bcef 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java @@ -14,20 +14,12 @@ package io.trino.plugin.iceberg.functions; import com.google.inject.Inject; -import io.trino.plugin.base.classloader.ClassLoaderSafeTableFunctionProcessorProvider; import io.trino.plugin.base.classloader.ClassLoaderSafeTableFunctionProcessorProviderFactory; -import io.trino.plugin.base.classloader.ClassLoaderSafeTableFunctionSplitProcessor; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesFunctionHandle; import io.trino.plugin.iceberg.functions.tablechanges.TableChangesFunctionProcessorProviderFactory; -import io.trino.plugin.iceberg.functions.tables.IcebergTablesFunction; -import io.trino.spi.classloader.ThreadContextClassLoader; -import io.trino.spi.connector.ConnectorSession; -import io.trino.spi.connector.ConnectorSplit; import io.trino.spi.function.FunctionProvider; import io.trino.spi.function.table.ConnectorTableFunctionHandle; -import io.trino.spi.function.table.TableFunctionProcessorProvider; import io.trino.spi.function.table.TableFunctionProcessorProviderFactory; -import io.trino.spi.function.table.TableFunctionSplitProcessor; import static java.util.Objects.requireNonNull; @@ -48,28 +40,6 @@ public TableFunctionProcessorProviderFactory getTableFunctionProcessorProviderFa if (functionHandle instanceof TableChangesFunctionHandle) { return new ClassLoaderSafeTableFunctionProcessorProviderFactory(tableChangesFunctionProcessorProviderFactory, getClass().getClassLoader()); } - if (functionHandle instanceof IcebergTablesFunction.IcebergTables) { - ClassLoader classLoader = getClass().getClassLoader(); - return new TableFunctionProcessorProviderFactory() - { - @Override - public TableFunctionProcessorProvider createTableFunctionProcessorProvider() - { - try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { - return new ClassLoaderSafeTableFunctionProcessorProvider(new TableFunctionProcessorProvider() - { - @Override - public TableFunctionSplitProcessor getSplitProcessor(ConnectorSession session, ConnectorTableFunctionHandle handle, ConnectorSplit split) - { - return new ClassLoaderSafeTableFunctionSplitProcessor( - new IcebergTablesFunction.IcebergTablesProcessor(((IcebergTablesFunction.IcebergTables) split).tables()), - getClass().getClassLoader()); - } - }, classLoader); - } - } - }; - } throw new UnsupportedOperationException("Unsupported function: " + functionHandle); } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java deleted file mode 100644 index 345c527a6ecf..000000000000 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java +++ /dev/null @@ -1,129 +0,0 @@ -/* - * 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.functions.tables; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import io.airlift.slice.Slice; -import io.airlift.slice.Slices; -import io.trino.plugin.iceberg.catalog.TrinoCatalog; -import io.trino.plugin.iceberg.catalog.TrinoCatalogFactory; -import io.trino.spi.Page; -import io.trino.spi.block.VariableWidthBlockBuilder; -import io.trino.spi.connector.ConnectorAccessControl; -import io.trino.spi.connector.ConnectorSession; -import io.trino.spi.connector.ConnectorSplit; -import io.trino.spi.connector.ConnectorTransactionHandle; -import io.trino.spi.connector.SchemaTableName; -import io.trino.spi.function.table.AbstractConnectorTableFunction; -import io.trino.spi.function.table.Argument; -import io.trino.spi.function.table.ConnectorTableFunctionHandle; -import io.trino.spi.function.table.Descriptor; -import io.trino.spi.function.table.ScalarArgument; -import io.trino.spi.function.table.ScalarArgumentSpecification; -import io.trino.spi.function.table.TableFunctionAnalysis; -import io.trino.spi.function.table.TableFunctionProcessorState; -import io.trino.spi.function.table.TableFunctionSplitProcessor; - -import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.Set; - -import static com.google.common.collect.Iterables.getOnlyElement; -import static io.trino.spi.function.table.ReturnTypeSpecification.GenericTable.GENERIC_TABLE; -import static io.trino.spi.function.table.TableFunctionProcessorState.Finished.FINISHED; -import static io.trino.spi.function.table.TableFunctionProcessorState.Processed.produced; -import static io.trino.spi.type.VarcharType.VARCHAR; -import static java.util.Objects.requireNonNull; - -public class IcebergTablesFunction - extends AbstractConnectorTableFunction -{ - private static final String FUNCTION_NAME = "iceberg_tables"; - private static final String SCHEMA_NAME_VAR_NAME = "SCHEMA_NAME"; - - private final TrinoCatalogFactory trinoCatalogFactory; - - public IcebergTablesFunction(TrinoCatalogFactory trinoCatalogFactory) - { - super( - "system", - FUNCTION_NAME, - ImmutableList.of( - ScalarArgumentSpecification.builder() - .name(SCHEMA_NAME_VAR_NAME) - .type(VARCHAR) - .defaultValue(null) - .build()), - GENERIC_TABLE); - this.trinoCatalogFactory = requireNonNull(trinoCatalogFactory, "trinoCatalogFactory is null"); - } - - @Override - public TableFunctionAnalysis analyze(ConnectorSession session, ConnectorTransactionHandle transaction, Map arguments, ConnectorAccessControl accessControl) - { - ScalarArgument argument = (ScalarArgument) getOnlyElement(arguments.values()); - Optional schemaFilter = Optional.ofNullable(((Slice) argument.getValue())).map(Slice::toStringUtf8); - - TrinoCatalog catalog = trinoCatalogFactory.create(session.getIdentity()); - List tables = catalog.listIcebergTables(session, schemaFilter); - Set filtered = accessControl.filterTables(null, ImmutableSet.copyOf(tables)); - return TableFunctionAnalysis.builder() - .returnedType(new Descriptor(ImmutableList.of( - new Descriptor.Field("table_schema", Optional.of(VARCHAR)), - new Descriptor.Field("table_name", Optional.of(VARCHAR))))) - .handle(new IcebergTables(filtered)) - .build(); - } - - public record IcebergTables(Collection tables) - implements ConnectorTableFunctionHandle, ConnectorSplit - { - public IcebergTables - { - requireNonNull(tables, "tables is null"); - } - } - - public static class IcebergTablesProcessor - implements TableFunctionSplitProcessor - { - private final Collection tables; - private boolean finished; - - public IcebergTablesProcessor(Collection tables) - { - this.tables = requireNonNull(tables, "tables is null"); - } - - @Override - public TableFunctionProcessorState process() - { - if (finished) { - return FINISHED; - } - - VariableWidthBlockBuilder schema = VARCHAR.createBlockBuilder(null, tables.size()); - VariableWidthBlockBuilder tableName = VARCHAR.createBlockBuilder(null, tables.size()); - for (SchemaTableName table : tables) { - schema.writeEntry(Slices.utf8Slice(table.getSchemaName())); - tableName.writeEntry(Slices.utf8Slice(table.getTableName())); - } - finished = true; - return produced(new Page(tables.size(), schema.build(), tableName.build())); - } - } -} diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunctionProvider.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunctionProvider.java deleted file mode 100644 index a0d281423f04..000000000000 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunctionProvider.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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.functions.tables; - -import com.google.inject.Inject; -import com.google.inject.Provider; -import io.trino.plugin.base.classloader.ClassLoaderSafeConnectorTableFunction; -import io.trino.plugin.iceberg.catalog.TrinoCatalogFactory; -import io.trino.spi.function.table.ConnectorTableFunction; - -import static java.util.Objects.requireNonNull; - -public class IcebergTablesFunctionProvider - implements Provider -{ - private final TrinoCatalogFactory trinoCatalogFactory; - - @Inject - public IcebergTablesFunctionProvider(TrinoCatalogFactory trinoCatalogFactory) - { - this.trinoCatalogFactory = requireNonNull(trinoCatalogFactory, "trinoCatalogFactory is null"); - } - - @Override - public ConnectorTableFunction get() - { - return new ClassLoaderSafeConnectorTableFunction( - new IcebergTablesFunction(trinoCatalogFactory), - getClass().getClassLoader()); - } -} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java index 745894d2d2ff..c9fef25627a5 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java @@ -22,7 +22,6 @@ import io.trino.filesystem.TrinoFileSystem; import io.trino.plugin.iceberg.fileio.ForwardingFileIo; import io.trino.testing.BaseConnectorSmokeTest; -import io.trino.testing.QueryRunner; import io.trino.testing.TestingConnectorBehavior; import io.trino.testing.sql.TestTable; import org.apache.iceberg.FileFormat; @@ -62,7 +61,6 @@ import static java.util.Objects.requireNonNull; import static java.util.concurrent.Executors.newFixedThreadPool; import static java.util.concurrent.TimeUnit.SECONDS; -import static java.util.stream.Collectors.joining; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; @@ -837,56 +835,6 @@ public void testCreateOrReplaceWithTableChangesFunction() } } - @Test - public void testIcebergTablesFunction() - throws Exception - { - String schemaName = getSession().getSchema().orElseThrow(); - String firstSchema = "first_schema_" + randomNameSuffix(); - String secondSchema = "second_schema_" + randomNameSuffix(); - String firstSchemaLocation = schemaPath().replaceAll(schemaName, firstSchema); - String secondSchemaLocation = schemaPath().replaceAll(schemaName, secondSchema); - assertQuerySucceeds("CREATE SCHEMA " + firstSchema + " WITH (location = '%s')".formatted(firstSchemaLocation)); - assertQuerySucceeds("CREATE SCHEMA " + secondSchema + " WITH (location = '%s')".formatted(secondSchemaLocation)); - QueryRunner queryRunner = getQueryRunner(); - Session firstSchemaSession = Session.builder(queryRunner.getDefaultSession()).setSchema(firstSchema).build(); - Session secondSchemaSession = Session.builder(queryRunner.getDefaultSession()).setSchema(secondSchema).build(); - - try (TestTable _ = new TestTable( - sql -> getQueryRunner().execute(firstSchemaSession, sql), - "first_schema_table1_", - "(id int)"); - TestTable _ = new TestTable( - sql -> getQueryRunner().execute(firstSchemaSession, sql), - "first_schema_table2_", - "(id int)"); - TestTable secondSchemaTable = new TestTable( - sql -> queryRunner.execute(secondSchemaSession, sql), - "second_schema_table_", - "(id int)"); - AutoCloseable _ = createAdditionalTables(firstSchema)) { - String firstSchemaTablesValues = "VALUES " + getQueryRunner() - .execute("SELECT table_schema, table_name FROM iceberg.information_schema.tables WHERE table_schema='%s'".formatted(firstSchema)) - .getMaterializedRows().stream() - .map(row -> "('%s', '%s')".formatted(row.getField(0), row.getField(1))) - .collect(joining(", ")); - String bothSchemasTablesValues = firstSchemaTablesValues + ", ('%s', '%s')".formatted(secondSchema, secondSchemaTable.getName()); - assertQuery("SELECT * FROM TABLE(iceberg.system.iceberg_tables(SCHEMA_NAME => '%s'))".formatted(firstSchema), firstSchemaTablesValues); - assertQuery("SELECT * FROM TABLE(iceberg.system.iceberg_tables(null)) WHERE table_schema = '%s'".formatted(firstSchema), firstSchemaTablesValues); - assertQuery("SELECT * FROM TABLE(iceberg.system.iceberg_tables()) WHERE table_schema in ('%s', '%s')".formatted(firstSchema, secondSchema), bothSchemasTablesValues); - assertQuery("SELECT * FROM TABLE(iceberg.system.iceberg_tables(null)) WHERE table_schema in ('%s', '%s')".formatted(firstSchema, secondSchema), bothSchemasTablesValues); - } - finally { - assertQuerySucceeds("DROP SCHEMA " + firstSchema); - assertQuerySucceeds("DROP SCHEMA " + secondSchema); - } - } - - protected AutoCloseable createAdditionalTables(String schema) - { - return () -> {}; - } - private long getMostRecentSnapshotId(String tableName) { return (long) Iterables.getOnlyElement(getQueryRunner().execute(format("SELECT snapshot_id FROM \"%s$snapshots\" ORDER BY committed_at DESC LIMIT 1", tableName)) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java index 91474fa6fa8c..3144bd56bb02 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java @@ -13,14 +13,10 @@ */ package io.trino.plugin.iceberg; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.minio.messages.Event; import io.trino.Session; -import io.trino.metastore.Column; import io.trino.metastore.HiveMetastore; -import io.trino.metastore.HiveType; -import io.trino.metastore.Table; import io.trino.plugin.hive.containers.Hive3MinioDataLake; import io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore; import io.trino.testing.QueryRunner; @@ -32,25 +28,18 @@ import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static io.trino.metastore.PrincipalPrivileges.NO_PRIVILEGES; -import static io.trino.plugin.hive.TableType.EXTERNAL_TABLE; import static io.trino.plugin.hive.TestingThriftHiveMetastoreBuilder.testingThriftHiveMetastoreBuilder; -import static io.trino.plugin.iceberg.IcebergTestUtils.getHiveMetastore; -import static io.trino.plugin.iceberg.catalog.AbstractIcebergTableOperations.ICEBERG_METASTORE_STORAGE_FORMAT; import static io.trino.testing.TestingNames.randomNameSuffix; import static io.trino.testing.containers.Minio.MINIO_ACCESS_KEY; import static io.trino.testing.containers.Minio.MINIO_REGION; import static io.trino.testing.containers.Minio.MINIO_SECRET_KEY; import static java.lang.String.format; import static java.util.Locale.ENGLISH; -import static org.apache.iceberg.BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE; -import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD; @@ -244,25 +233,6 @@ public void testPathContainsSpecialCharacter() assertUpdate("DROP TABLE " + tableName); } - @Override - protected AutoCloseable createAdditionalTables(String schema) - { - HiveMetastore metastore = getHiveMetastore(getQueryRunner()); - // simulate iceberg table created by spark with lowercase table type - Table lowerCaseTableType = io.trino.metastore.Table.builder() - .setDatabaseName(schema) - .setTableName("lowercase_type_" + randomNameSuffix()) - .setOwner(Optional.empty()) - .setDataColumns(ImmutableList.of(new Column("id", HiveType.HIVE_STRING, Optional.empty(), ImmutableMap.of()))) - .setTableType(EXTERNAL_TABLE.name()) - .withStorage(storage -> storage.setStorageFormat(ICEBERG_METASTORE_STORAGE_FORMAT)) - .setParameter("EXTERNAL", "TRUE") - .setParameter(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE.toLowerCase(ENGLISH)) - .build(); - metastore.createTable(lowerCaseTableType, NO_PRIVILEGES); - return () -> metastore.dropTable(lowerCaseTableType.getDatabaseName(), lowerCaseTableType.getTableName(), true); - } - private String onMetastore(@Language("SQL") String sql) { return hiveMinioDataLake.getHiveHadoop().runOnMetastore(sql); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseSharedMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseSharedMetastoreTest.java index b755bcea9859..d059623c8294 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseSharedMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseSharedMetastoreTest.java @@ -152,13 +152,6 @@ public void testShowSchemas() assertThat(showCreateIcebergWithRedirectionsSchema).isEqualTo(getExpectedIcebergCreateSchema("iceberg_with_redirections")); } - @Test - public void testIcebergTablesFunction() - { - assertQuery("SELECT * FROM TABLE(iceberg.system.iceberg_tables(SCHEMA_NAME => '%s'))".formatted(tpchSchema), "VALUES ('%s', 'nation')".formatted(tpchSchema)); - assertQuery("SELECT * FROM TABLE(iceberg_with_redirections.system.iceberg_tables(SCHEMA_NAME => '%s'))".formatted(tpchSchema), "VALUES ('%s', 'nation')".formatted(tpchSchema)); - } - @Test public void testTimeTravelWithRedirection() throws InterruptedException diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSharedHiveThriftMetastore.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSharedHiveThriftMetastore.java deleted file mode 100644 index 6228db8ed7c6..000000000000 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestSharedHiveThriftMetastore.java +++ /dev/null @@ -1,181 +0,0 @@ -/* - * 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; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import io.trino.Session; -import io.trino.plugin.hive.TestingHivePlugin; -import io.trino.plugin.hive.containers.Hive3MinioDataLake; -import io.trino.plugin.hive.containers.HiveMinioDataLake; -import io.trino.plugin.tpch.TpchPlugin; -import io.trino.testing.DistributedQueryRunner; -import io.trino.testing.QueryRunner; -import io.trino.tpch.TpchTable; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.TestInstance; -import org.junit.jupiter.api.parallel.Execution; - -import java.nio.file.Path; -import java.util.Map; - -import static io.trino.plugin.iceberg.IcebergQueryRunner.ICEBERG_CATALOG; -import static io.trino.plugin.tpch.TpchMetadata.TINY_SCHEMA_NAME; -import static io.trino.testing.QueryAssertions.copyTpchTables; -import static io.trino.testing.TestingNames.randomNameSuffix; -import static io.trino.testing.TestingSession.testSessionBuilder; -import static io.trino.testing.containers.Minio.MINIO_ACCESS_KEY; -import static io.trino.testing.containers.Minio.MINIO_REGION; -import static io.trino.testing.containers.Minio.MINIO_SECRET_KEY; -import static java.lang.String.format; -import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; -import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; - -@TestInstance(PER_CLASS) -@Execution(CONCURRENT) -public class TestSharedHiveThriftMetastore - extends BaseSharedMetastoreTest -{ - private static final String HIVE_CATALOG = "hive"; - private String bucketName; - - @Override - protected QueryRunner createQueryRunner() - throws Exception - { - bucketName = "test-iceberg-shared-metastore" + randomNameSuffix(); - HiveMinioDataLake hiveMinioDataLake = closeAfterClass(new Hive3MinioDataLake(bucketName)); - hiveMinioDataLake.start(); - - Session icebergSession = testSessionBuilder() - .setCatalog(ICEBERG_CATALOG) - .setSchema(tpchSchema) - .build(); - Session hiveSession = testSessionBuilder() - .setCatalog(HIVE_CATALOG) - .setSchema(tpchSchema) - .build(); - - QueryRunner queryRunner = DistributedQueryRunner.builder(icebergSession).build(); - - queryRunner.installPlugin(new TpchPlugin()); - queryRunner.createCatalog("tpch", "tpch"); - - Path dataDirectory = queryRunner.getCoordinator().getBaseDataDir().resolve("iceberg_data"); - dataDirectory.toFile().deleteOnExit(); - - queryRunner.installPlugin(new IcebergPlugin()); - queryRunner.createCatalog( - ICEBERG_CATALOG, - "iceberg", - ImmutableMap.builder() - .put("iceberg.catalog.type", "HIVE_METASTORE") - .put("hive.metastore.uri", hiveMinioDataLake.getHiveHadoop().getHiveMetastoreEndpoint().toString()) - .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout - .put("fs.hadoop.enabled", "false") - .put("fs.native-s3.enabled", "true") - .put("s3.aws-access-key", MINIO_ACCESS_KEY) - .put("s3.aws-secret-key", MINIO_SECRET_KEY) - .put("s3.region", MINIO_REGION) - .put("s3.endpoint", hiveMinioDataLake.getMinio().getMinioAddress()) - .put("s3.path-style-access", "true") - .put("s3.streaming.part-size", "5MB") // minimize memory usage - .put("s3.max-connections", "2") // verify no leaks - .put("iceberg.register-table-procedure.enabled", "true") - .put("iceberg.writer-sort-buffer-size", "1MB") - .buildOrThrow()); - queryRunner.createCatalog( - "iceberg_with_redirections", - "iceberg", - ImmutableMap.builder() - .put("iceberg.catalog.type", "HIVE_METASTORE") - .put("hive.metastore.uri", hiveMinioDataLake.getHiveHadoop().getHiveMetastoreEndpoint().toString()) - .put("hive.metastore.thrift.client.read-timeout", "1m") // read timed out sometimes happens with the default timeout - .put("fs.hadoop.enabled", "false") - .put("fs.native-s3.enabled", "true") - .put("s3.aws-access-key", MINIO_ACCESS_KEY) - .put("s3.aws-secret-key", MINIO_SECRET_KEY) - .put("s3.region", MINIO_REGION) - .put("s3.endpoint", hiveMinioDataLake.getMinio().getMinioAddress()) - .put("s3.path-style-access", "true") - .put("s3.streaming.part-size", "5MB") // minimize memory usage - .put("s3.max-connections", "2") // verify no leaks - .put("iceberg.register-table-procedure.enabled", "true") - .put("iceberg.writer-sort-buffer-size", "1MB") - .put("iceberg.hive-catalog-name", "hive") - .buildOrThrow()); - - queryRunner.installPlugin(new TestingHivePlugin(dataDirectory)); - Map hiveProperties = ImmutableMap.builder() - .put("hive.metastore", "thrift") - .put("hive.metastore.uri", hiveMinioDataLake.getHiveHadoop().getHiveMetastoreEndpoint().toString()) - .put("fs.hadoop.enabled", "false") - .put("fs.native-s3.enabled", "true") - .put("s3.aws-access-key", MINIO_ACCESS_KEY) - .put("s3.aws-secret-key", MINIO_SECRET_KEY) - .put("s3.region", MINIO_REGION) - .put("s3.endpoint", hiveMinioDataLake.getMinio().getMinioAddress()) - .put("s3.path-style-access", "true") - .put("s3.streaming.part-size", "5MB") - .put("hive.max-partitions-per-scan", "1000") - .put("hive.max-partitions-for-eager-load", "1000") - .put("hive.security", "allow-all") - .buildOrThrow(); - queryRunner.createCatalog(HIVE_CATALOG, "hive", hiveProperties); - queryRunner.createCatalog( - "hive_with_redirections", - "hive", - ImmutableMap.builder() - .putAll(hiveProperties).put("hive.iceberg-catalog-name", "iceberg") - .buildOrThrow()); - - queryRunner.execute("CREATE SCHEMA " + tpchSchema + " WITH (location = 's3://" + bucketName + "/" + tpchSchema + "')"); - copyTpchTables(queryRunner, "tpch", TINY_SCHEMA_NAME, icebergSession, ImmutableList.of(TpchTable.NATION)); - copyTpchTables(queryRunner, "tpch", TINY_SCHEMA_NAME, hiveSession, ImmutableList.of(TpchTable.REGION)); - queryRunner.execute("CREATE SCHEMA " + testSchema + " WITH (location = 's3://" + bucketName + "/" + testSchema + "')"); - - return queryRunner; - } - - @AfterAll - public void cleanup() - { - assertQuerySucceeds("DROP TABLE IF EXISTS hive." + tpchSchema + ".region"); - assertQuerySucceeds("DROP TABLE IF EXISTS iceberg." + tpchSchema + ".nation"); - assertQuerySucceeds("DROP SCHEMA IF EXISTS hive." + tpchSchema); - assertQuerySucceeds("DROP SCHEMA IF EXISTS hive." + testSchema); - } - - @Override - protected String getExpectedHiveCreateSchema(String catalogName) - { - return """ - CREATE SCHEMA %s.%s - WITH ( - location = 's3://%s/%s' - )""" - .formatted(catalogName, tpchSchema, bucketName, tpchSchema); - } - - @Override - protected String getExpectedIcebergCreateSchema(String catalogName) - { - String expectedIcebergCreateSchema = "CREATE SCHEMA %s.%s\n" + - "AUTHORIZATION USER user\n" + - "WITH (\n" + - " location = 's3://%s/%s'\n" + - ")"; - return format(expectedIcebergCreateSchema, catalogName, tpchSchema, bucketName, tpchSchema); - } -} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergUnityRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergUnityRestCatalogConnectorSmokeTest.java index 01f87a4b3d73..a6cdc1793d2b 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergUnityRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergUnityRestCatalogConnectorSmokeTest.java @@ -95,7 +95,7 @@ protected String getMetadataLocation(String tableName) @Override protected String schemaPath() { - return format("%s/%s", warehouseLocation, getSession().getSchema().orElseThrow()); + return format("%s/%s", warehouseLocation, getSession().getSchema()); } @Override @@ -470,7 +470,7 @@ public void testDropTableWithMissingDataFile() public void testDropTableWithNonExistentTableLocation() { assertThatThrownBy(super::testDropTableWithNonExistentTableLocation) - .hasStackTraceContaining("Access Denied"); + .hasMessageContaining("Access Denied"); } @Test @@ -520,12 +520,4 @@ public void testTruncateTable() assertThatThrownBy(super::testTruncateTable) .hasMessageContaining("Access Denied"); } - - @Test - @Override - public void testIcebergTablesFunction() - { - assertThatThrownBy(super::testIcebergTablesFunction) - .hasStackTraceContaining("Access Denied"); - } } From f2accbc3224cfd1b9f94585317ce95a694168c27 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 2/6] Revert "Add TrinoCatalog.listIcebergTables" This reverts commit bc385e70622d0a54837bcd8253c7770c13f8519d. --- .../plugin/iceberg/catalog/TrinoCatalog.java | 2 -- .../catalog/glue/TrinoGlueCatalog.java | 17 -------------- .../iceberg/catalog/hms/TrinoHiveCatalog.java | 23 ------------------- .../catalog/jdbc/TrinoJdbcCatalog.java | 21 ----------------- .../catalog/nessie/TrinoNessieCatalog.java | 8 ------- .../catalog/rest/TrinoRestCatalog.java | 15 ------------ .../snowflake/TrinoSnowflakeCatalog.java | 9 -------- .../iceberg/catalog/BaseTrinoCatalogTest.java | 7 ------ 8 files changed, 102 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java index c7370d65d810..005760040b1b 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java @@ -83,8 +83,6 @@ default Optional getNamespaceSeparator() List listTables(ConnectorSession session, Optional namespace); - List listIcebergTables(ConnectorSession session, Optional namespace); - default List listViews(ConnectorSession session, Optional namespace) { return listTables(session, namespace).stream() diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java index 6c8a3dfa44bb..7b5e4cb67d1b 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java @@ -373,26 +373,9 @@ public void renameNamespace(ConnectorSession session, String source, String targ @Override public List listTables(ConnectorSession session, Optional namespace) - { - return listTables(session, namespace, _ -> true); - } - - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - return listTables(session, namespace, table -> isIcebergTable(getTableParameters(table))).stream() - .map(TableInfo::tableName) - .collect(toImmutableList()); - } - - private List listTables( - ConnectorSession session, - Optional namespace, - Predicate tablePredicate) { List>> tasks = listNamespaces(session, namespace).stream() .map(glueNamespace -> (Callable>) () -> getGlueTablesWithExceptionHandling(glueNamespace) - .filter(tablePredicate) .map(table -> mapToTableInfo(glueNamespace, table)) .collect(toImmutableList())) .collect(toImmutableList()); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java index 908d7cc6dde9..b450063dba6f 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java @@ -16,7 +16,6 @@ import com.google.common.cache.Cache; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.UncheckedExecutionException; import io.airlift.log.Logger; import io.trino.cache.EvictableCacheBuilder; @@ -376,28 +375,6 @@ public List listTables(ConnectorSession session, Optional nam } } - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - List>> tasks = listNamespaces(session, namespace).stream() - .map(schema -> (Callable>) () -> metastore.getTableNamesWithParameters(schema, TABLE_TYPE_PROP, ImmutableSet.of( - // Get tables with parameter table_type set to "ICEBERG" or "iceberg". This is required because - // Trino uses lowercase value whereas Spark and Flink use uppercase. - ICEBERG_TABLE_TYPE_VALUE.toLowerCase(ENGLISH), - ICEBERG_TABLE_TYPE_VALUE.toUpperCase(ENGLISH))).stream() - .map(tableName -> new SchemaTableName(schema, tableName)) - .collect(toImmutableList())) - .collect(toImmutableList()); - try { - return processWithAdditionalThreads(tasks, metadataFetchingExecutor).stream() - .flatMap(Collection::stream) - .collect(toImmutableList()); - } - catch (ExecutionException e) { - throw new RuntimeException(e.getCause()); - } - } - @Override public Optional> streamRelationColumns( ConnectorSession session, diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java index 0e5c401c8ba5..1d8daf1effd1 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java @@ -60,7 +60,6 @@ import org.apache.iceberg.view.ViewVersion; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -201,26 +200,6 @@ public List listTables(ConnectorSession session, Optional nam return ImmutableList.copyOf(tablesListBuilder.values()); } - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - List namespaces = listNamespaces(session, namespace); - - // Build as a set and convert to list for removing duplicate entries due to case difference - Set tablesListBuilder = new HashSet<>(); - for (String schemaName : namespaces) { - try { - listTableIdentifiers(schemaName, () -> jdbcCatalog.listTables(Namespace.of(schemaName))).stream() - .map(tableId -> SchemaTableName.schemaTableName(schemaName, tableId.name())) - .forEach(tablesListBuilder::add); - } - catch (NoSuchNamespaceException e) { - // Namespace may have been deleted - } - } - return ImmutableList.copyOf(tablesListBuilder); - } - @Override public List listViews(ConnectorSession session, Optional namespace) { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/TrinoNessieCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/TrinoNessieCatalog.java index ed7be861fd9f..240e41f4cdd1 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/TrinoNessieCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/nessie/TrinoNessieCatalog.java @@ -170,14 +170,6 @@ public List listTables(ConnectorSession session, Optional nam .collect(toImmutableList()); } - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - return listTables(session, namespace).stream() - .map(TableInfo::tableName) - .collect(toImmutableList()); - } - @Override public Optional> streamRelationColumns( ConnectorSession session, diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java index d72f75d3f334..40037baed704 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java @@ -261,21 +261,6 @@ public List listTables(ConnectorSession session, Optional nam return tables.build(); } - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - SessionContext sessionContext = convert(session); - List namespaces = listNamespaces(session, namespace); - - ImmutableList.Builder tables = ImmutableList.builder(); - for (Namespace restNamespace : namespaces) { - listTableIdentifiers(restNamespace, () -> restSessionCatalog.listTables(sessionContext, toRemoteNamespace(session, restNamespace))).stream() - .map(id -> SchemaTableName.schemaTableName(toSchemaName(id.namespace()), id.name())) - .forEach(tables::add); - } - return tables.build(); - } - @Override public List listViews(ConnectorSession session, Optional namespace) { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java index 2f58396e6c69..fb901d4890b0 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java @@ -58,7 +58,6 @@ import java.util.stream.Stream; import static com.google.common.base.Throwables.throwIfUnchecked; -import static com.google.common.collect.ImmutableList.toImmutableList; import static io.trino.cache.CacheUtils.uncheckedCacheGet; import static io.trino.plugin.iceberg.IcebergUtil.getIcebergTableWithMetadata; import static io.trino.plugin.iceberg.IcebergUtil.quotedTableName; @@ -172,14 +171,6 @@ public List listTables(ConnectorSession session, Optional nam .toList(); } - @Override - public List listIcebergTables(ConnectorSession session, Optional namespace) - { - return listTables(session, namespace).stream() - .map(TableInfo::tableName) - .collect(toImmutableList()); - } - @Override public Optional> streamRelationColumns( ConnectorSession session, diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java index e2ae7af4c66b..1cf15d256401 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java @@ -450,9 +450,6 @@ public void testListTables() .add(new TableInfo(table1, TABLE)) .add(new TableInfo(table2, TABLE)); - ImmutableList.Builder icebergTables = ImmutableList.builder() - .add(table1) - .add(table2); SchemaTableName view = new SchemaTableName(ns2, "view"); try { catalog.createView( @@ -496,7 +493,6 @@ public void testListTables() createExternalIcebergTable(catalog, ns2, closer).ifPresent(table -> { allTables.add(new TableInfo(table, TABLE)); - icebergTables.add(table); }); createExternalNonIcebergTable(catalog, ns2, closer).ifPresent(table -> { allTables.add(new TableInfo(table, TABLE)); @@ -504,13 +500,10 @@ public void testListTables() // No namespace provided, all tables across all namespaces should be returned assertThat(catalog.listTables(SESSION, Optional.empty())).containsAll(allTables.build()); - assertThat(catalog.listIcebergTables(SESSION, Optional.empty())).containsAll(icebergTables.build()); // Namespace is provided and exists assertThat(catalog.listTables(SESSION, Optional.of(ns1))).containsExactly(new TableInfo(table1, TABLE)); - assertThat(catalog.listIcebergTables(SESSION, Optional.of(ns1))).containsExactly(table1); // Namespace is provided and does not exist assertThat(catalog.listTables(SESSION, Optional.of("non_existing"))).isEmpty(); - assertThat(catalog.listIcebergTables(SESSION, Optional.of("non_existing"))).isEmpty(); } } From a2e8e4802206add7a9f827375e47c19beb44b4fc Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 3/6] Revert "Add HiveMetastore.getTableNamesWithParameters" This reverts commit 8cd060d86ae335ab3cf6c8547931a32e188e9725. --- .../io/trino/metastore/HiveMetastore.java | 6 --- .../tracing/TracingHiveMetastore.java | 15 ------- .../metastore/cache/CachingHiveMetastore.java | 38 ---------------- .../cache/SharedHiveMetastoreCache.java | 7 --- .../metastore/file/FileHiveMetastore.java | 17 ++----- .../metastore/glue/GlueHiveMetastore.java | 16 +------ .../metastore/glue/v1/GlueHiveMetastore.java | 19 -------- .../thrift/BridgingHiveMetastore.java | 7 --- .../DefaultThriftMetastoreClientFactory.java | 2 - .../FailureAwareThriftMetastoreClient.java | 8 ---- .../HttpThriftMetastoreClientFactory.java | 2 - .../metastore/thrift/ThriftHiveMetastore.java | 24 ---------- .../thrift/ThriftHiveMetastoreClient.java | 45 ------------------- .../metastore/thrift/ThriftMetastore.java | 2 - .../thrift/ThriftMetastoreClient.java | 4 -- .../plugin/hive/TestHiveMetadataListing.java | 7 --- .../thrift/MockThriftMetastoreClient.java | 7 --- .../thrift/TestThriftHiveMetastoreClient.java | 1 - 18 files changed, 5 insertions(+), 222 deletions(-) diff --git a/lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java b/lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java index ff2f44ee0946..8d06dadf5d6b 100644 --- a/lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java +++ b/lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java @@ -13,7 +13,6 @@ */ package io.trino.metastore; -import com.google.common.collect.ImmutableSet; import io.trino.metastore.HivePrivilegeInfo.HivePrivilege; import io.trino.spi.TrinoException; import io.trino.spi.connector.SchemaTableName; @@ -68,11 +67,6 @@ default boolean useSparkTableStatistics() List getTables(String databaseName); - /** - * @param parameterValues is using ImmutableSet to mark that this api does not support filtering by null parameter value. - */ - List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues); - void createDatabase(Database database); void dropDatabase(String databaseName, boolean deleteData); diff --git a/lib/trino-metastore/src/main/java/io/trino/metastore/tracing/TracingHiveMetastore.java b/lib/trino-metastore/src/main/java/io/trino/metastore/tracing/TracingHiveMetastore.java index 4549a69af5cc..e8c6f56631ca 100644 --- a/lib/trino-metastore/src/main/java/io/trino/metastore/tracing/TracingHiveMetastore.java +++ b/lib/trino-metastore/src/main/java/io/trino/metastore/tracing/TracingHiveMetastore.java @@ -13,7 +13,6 @@ */ package io.trino.metastore.tracing; -import com.google.common.collect.ImmutableSet; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.Tracer; import io.trino.metastore.AcidOperation; @@ -165,20 +164,6 @@ public List getTables(String databaseName) }); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - Span span = tracer.spanBuilder("HiveMetastore.getTableNamesWithParameters") - .setAttribute(SCHEMA, databaseName) - .setAttribute(TABLE, parameterKey) - .startSpan(); - return withTracing(span, () -> { - List tables = delegate.getTableNamesWithParameters(databaseName, parameterKey, parameterValues); - span.setAttribute(TABLE_RESPONSE_COUNT, tables.size()); - return tables; - }); - } - @Override public void createDatabase(Database database) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java index f3cf6ee5db16..bcee3aa656d5 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java @@ -123,7 +123,6 @@ public enum ObjectType private final LoadingCache> tableCache; private final LoadingCache> tablesCacheNew; private final Cache>> tableColumnStatisticsCache; - private final LoadingCache> tableNamesWithParametersCache; private final Cache>> partitionStatisticsCache; private final Cache>> partitionCache; private final LoadingCache>> partitionFilterCache; @@ -207,7 +206,6 @@ private CachingHiveMetastore( tablesCacheNew = cacheFactory.buildCache(this::loadTablesNew); tableColumnStatisticsCache = statsCacheFactory.buildCache(this::refreshTableColumnStatistics); tableCache = cacheFactory.buildCache(this::loadTable); - tableNamesWithParametersCache = cacheFactory.buildCache(this::loadTablesMatchingParameter); tablePrivilegesCache = cacheFactory.buildCache(key -> loadTablePrivileges(key.database(), key.table(), key.owner(), key.principal())); rolesCache = cacheFactory.buildCache(_ -> loadRoles()); roleGrantsCache = cacheFactory.buildCache(this::loadRoleGrants); @@ -225,7 +223,6 @@ public void flushCache() tablesCacheNew.invalidateAll(); databaseCache.invalidateAll(); tableCache.invalidateAll(); - tableNamesWithParametersCache.invalidateAll(); partitionCache.invalidateAll(); partitionFilterCache.invalidateAll(); tablePrivilegesCache.invalidateAll(); @@ -568,18 +565,6 @@ private List loadTablesNew(String databaseName) return delegate.getTables(databaseName); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - TablesWithParameterCacheKey key = new TablesWithParameterCacheKey(databaseName, parameterKey, parameterValues); - return get(tableNamesWithParametersCache, key); - } - - private List loadTablesMatchingParameter(TablesWithParameterCacheKey key) - { - return delegate.getTableNamesWithParameters(key.databaseName(), key.parameterKey(), key.parameterValues()); - } - @Override public void createDatabase(Database database) { @@ -748,7 +733,6 @@ public void invalidateTable(String databaseName, String tableName) HiveTableName hiveTableName = new HiveTableName(databaseName, tableName); tableCache.invalidate(hiveTableName); tablesCacheNew.invalidate(databaseName); - tableNamesWithParametersCache.invalidateAll(); invalidateAllIf(tablePrivilegesCache, userTableKey -> userTableKey.matches(databaseName, tableName)); tableColumnStatisticsCache.invalidate(hiveTableName); invalidatePartitionCache(databaseName, tableName); @@ -1169,16 +1153,6 @@ private static Cache> buildBulkCache( return cacheBuilder.build(); } - record TablesWithParameterCacheKey(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - TablesWithParameterCacheKey - { - requireNonNull(databaseName, "databaseName is null"); - requireNonNull(parameterKey, "parameterKey is null"); - requireNonNull(parameterValues, "parameterValues is null"); - } - } - record UserTableKey(Optional principal, String database, String table, Optional owner) { UserTableKey @@ -1227,13 +1201,6 @@ public CacheStatsMBean getTableNamesStats() return new CacheStatsMBean(tablesCacheNew); } - @Managed - @Nested - public CacheStatsMBean getTableWithParameterStats() - { - return new CacheStatsMBean(tableNamesWithParametersCache); - } - @Managed @Nested public CacheStatsMBean getTableColumnStatisticsStats() @@ -1308,11 +1275,6 @@ LoadingCache> getTableCache() return tableCache; } - LoadingCache> getTableNamesWithParametersCache() - { - return tableNamesWithParametersCache; - } - public LoadingCache> getTablesCacheNew() { return tablesCacheNew; diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/SharedHiveMetastoreCache.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/SharedHiveMetastoreCache.java index 364d25a02047..b8c50af89a55 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/SharedHiveMetastoreCache.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/SharedHiveMetastoreCache.java @@ -261,13 +261,6 @@ public AggregateCacheStatsMBean getTablesStats() return new AggregateCacheStatsMBean(CachingHiveMetastore::getTablesCacheNew); } - @Managed - @Nested - public AggregateCacheStatsMBean getTableWithParameterStats() - { - return new AggregateCacheStatsMBean(CachingHiveMetastore::getTableNamesWithParametersCache); - } - @Managed @Nested public AggregateCacheStatsMBean getTableColumnStatisticsCache() diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java index 7f6efda69f4b..89343de0c71a 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java @@ -83,7 +83,6 @@ import java.util.OptionalLong; import java.util.Set; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; import static com.google.common.base.Preconditions.checkArgument; @@ -178,7 +177,7 @@ public FileHiveMetastore(NodeVersion nodeVersion, TrinoFileSystemFactory fileSys listTablesCache = EvictableCacheBuilder.newBuilder() .expireAfterWrite(10, SECONDS) - .build(CacheLoader.from(databaseName -> doListAllTables(databaseName, _ -> true))); + .build(CacheLoader.from(this::doListAllTables)); } @Override @@ -533,16 +532,7 @@ private List listAllTables(String databaseName) return listTablesCache.getUnchecked(databaseName); } - @Override - public synchronized List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - requireNonNull(parameterKey, "parameterKey is null"); - return doListAllTables(databaseName, table -> parameterValues.contains(table.getParameters().get(parameterKey))).stream() - .map(tableInfo -> tableInfo.tableName().getTableName()) - .collect(toImmutableList()); - } - - private synchronized List doListAllTables(String databaseName, Predicate tableMetadataPredicate) + private synchronized List doListAllTables(String databaseName) { requireNonNull(databaseName, "databaseName is null"); @@ -567,8 +557,7 @@ private synchronized List doListAllTables(String databaseName, Predic Location schemaFileLocation = subdirectory.appendPath(TRINO_SCHEMA_FILE_NAME_SUFFIX); readFile("table schema", schemaFileLocation, tableCodec).ifPresent(tableMetadata -> { checkVersion(tableMetadata.getWriterVersion()); - if ((hideDeltaLakeTables && DELTA_LAKE_PROVIDER.equals(tableMetadata.getParameters().get(SPARK_TABLE_PROVIDER_KEY))) - || !tableMetadataPredicate.test(tableMetadata)) { + if (hideDeltaLakeTables && DELTA_LAKE_PROVIDER.equals(tableMetadata.getParameters().get(SPARK_TABLE_PROVIDER_KEY))) { return; } tables.add(new TableInfo( diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java index eee2a234a016..59e05ac8c334 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java @@ -412,21 +412,10 @@ public void setDatabaseOwner(String databaseName, HivePrincipal principal) @Override public List getTables(String databaseName) { - return glueCache.getTables(databaseName, cacheTable -> getTablesInternal(cacheTable, databaseName, _ -> true)); + return glueCache.getTables(databaseName, cacheTable -> getTablesInternal(cacheTable, databaseName)); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - return getTablesInternal( - _ -> {}, - databaseName, - table -> table.parameters() != null && parameterValues.contains(table.parameters().get(parameterKey))).stream() - .map(tableInfo -> tableInfo.tableName().getTableName()) - .collect(toImmutableList()); - } - - private List getTablesInternal(Consumer cacheTable, String databaseName, Predicate filter) + private List getTablesInternal(Consumer
cacheTable, String databaseName) { try { ImmutableList glueTables = stats.getGetTables() @@ -436,7 +425,6 @@ private List getTablesInternal(Consumer
cacheTable, String dat .map(GetTablesResponse::tableList) .flatMap(List::stream)) .filter(tableVisibilityFilter) - .filter(filter) .collect(toImmutableList()); // Store only valid tables in cache diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastore.java index 5734df27bd79..a988769b58ee 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastore.java @@ -374,25 +374,6 @@ public List getTables(String databaseName) } } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - try { - return getGlueTables(databaseName) - .filter(tableFilter) - .filter(table -> parameterValues.contains(getTableParameters(table).get(parameterKey))) - .map(com.amazonaws.services.glue.model.Table::getName) - .collect(toImmutableList()); - } - catch (EntityNotFoundException | AccessDeniedException e) { - // database does not exist or permission denied - return ImmutableList.of(); - } - catch (AmazonServiceException e) { - throw new TrinoException(HIVE_METASTORE_ERROR, e); - } - } - @Override public Optional
getTable(String databaseName, String tableName) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java index 7d1efdafe889..826a6caf0c31 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java @@ -15,7 +15,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import io.trino.hive.thrift.metastore.FieldSchema; import io.trino.metastore.AcidOperation; import io.trino.metastore.AcidTransactionOwner; @@ -153,12 +152,6 @@ public List getTables(String databaseName) .collect(toImmutableList()); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - return delegate.getTableNamesWithParameters(databaseName, parameterKey, parameterValues); - } - @Override public void createDatabase(Database database) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java index a01a87b9e69f..ec0d33783f82 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java @@ -49,7 +49,6 @@ public class DefaultThriftMetastoreClientFactory private final MetastoreSupportsDateStatistics metastoreSupportsDateStatistics = new MetastoreSupportsDateStatistics(); private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); - private final AtomicInteger chosenTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); private final AtomicInteger chosenAlterTransactionalTableAlternative = new AtomicInteger(Integer.MAX_VALUE); private final AtomicInteger chosenAlterPartitionsAlternative = new AtomicInteger(Integer.MAX_VALUE); @@ -116,7 +115,6 @@ protected ThriftMetastoreClient create(TransportSupplier transportSupplier, Stri metastoreSupportsDateStatistics, true, chosenGetTableAlternative, - chosenTableParamAlternative, chosenAlterTransactionalTableAlternative, chosenAlterPartitionsAlternative); } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java index 0d9ee63183ab..ed57071e3389 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java @@ -37,7 +37,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Set; import static java.util.Objects.requireNonNull; @@ -93,13 +92,6 @@ public List getTableMeta(String databaseName) return runWithHandle(() -> delegate.getTableMeta(databaseName)); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues) - throws TException - { - return runWithHandle(() -> delegate.getTableNamesWithParameters(databaseName, parameterKey, parameterValues)); - } - @Override public void createDatabase(Database database) throws TException diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/HttpThriftMetastoreClientFactory.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/HttpThriftMetastoreClientFactory.java index ddd696fa9d54..52b0df41ca1b 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/HttpThriftMetastoreClientFactory.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/HttpThriftMetastoreClientFactory.java @@ -57,7 +57,6 @@ public class HttpThriftMetastoreClientFactory private final OpenTelemetry openTelemetry; private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); - private final AtomicInteger chosenGetTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); private final AtomicInteger chosenAlterTransactionalTableAlternative = new AtomicInteger(Integer.MAX_VALUE); private final AtomicInteger chosenAlterPartitionsAlternative = new AtomicInteger(Integer.MAX_VALUE); @@ -86,7 +85,6 @@ public ThriftMetastoreClient create(URI uri, Optional delegationToken) new MetastoreSupportsDateStatistics(), false, chosenGetTableAlternative, - chosenGetTableParamAlternative, chosenAlterTransactionalTableAlternative, chosenAlterPartitionsAlternative); } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java index 7b07889050cd..a7908d8e7de0 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java @@ -268,30 +268,6 @@ public List getTables(String databaseName) } } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues) - { - try { - return retry() - .stopOn(NoSuchObjectException.class) - .stopOnIllegalExceptions() - .run("getTableNamesWithParameters", () -> { - try (ThriftMetastoreClient client = createMetastoreClient()) { - return client.getTableNamesWithParameters(databaseName, parameterKey, parameterValues); - } - }); - } - catch (NoSuchObjectException e) { - return ImmutableList.of(); - } - catch (TException e) { - throw new TrinoException(HIVE_METASTORE_ERROR, e); - } - catch (Exception e) { - throw propagate(e); - } - } - @Override public Optional
getTable(String databaseName, String tableName) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java index 97c7a5fb3ead..acf253025ef5 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java @@ -78,10 +78,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; -import java.util.regex.Pattern; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Throwables.throwIfInstanceOf; @@ -92,7 +90,6 @@ import static com.google.common.reflect.Reflection.newProxy; import static io.trino.hive.thrift.metastore.GrantRevokeType.GRANT; import static io.trino.hive.thrift.metastore.GrantRevokeType.REVOKE; -import static io.trino.hive.thrift.metastore.hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS; import static io.trino.metastore.TableInfo.PRESTO_VIEW_COMMENT; import static io.trino.plugin.hive.TableType.VIRTUAL_VIEW; import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.NOT_SUPPORTED; @@ -102,7 +99,6 @@ import static io.trino.plugin.hive.metastore.thrift.TxnUtils.createValidTxnWriteIdList; import static java.lang.String.format; import static java.util.Objects.requireNonNull; -import static java.util.stream.Collectors.joining; import static org.apache.thrift.TApplicationException.UNKNOWN_METHOD; public class ThriftHiveMetastoreClient @@ -113,9 +109,6 @@ public class ThriftHiveMetastoreClient private static final String CATALOG_DB_SEPARATOR = "#"; private static final String DB_EMPTY_MARKER = "!"; - private static final Pattern TABLE_PARAMETER_SAFE_KEY_PATTERN = Pattern.compile("^[a-zA-Z_]+$"); - private static final Pattern TABLE_PARAMETER_SAFE_VALUE_PATTERN = Pattern.compile("^[a-zA-Z0-9\\s]*$"); - private final TransportSupplier transportSupplier; private TTransport transport; protected ThriftHiveMetastore.Iface client; @@ -124,7 +117,6 @@ public class ThriftHiveMetastoreClient private final MetastoreSupportsDateStatistics metastoreSupportsDateStatistics; private final boolean metastoreSupportsTableMeta; private final AtomicInteger chosenGetTableAlternative; - private final AtomicInteger chosenTableParamAlternative; private final AtomicInteger chosenAlterTransactionalTableAlternative; private final AtomicInteger chosenAlterPartitionsAlternative; private final Optional catalogName; @@ -136,7 +128,6 @@ public ThriftHiveMetastoreClient( MetastoreSupportsDateStatistics metastoreSupportsDateStatistics, boolean metastoreSupportsTableMeta, AtomicInteger chosenGetTableAlternative, - AtomicInteger chosenTableParamAlternative, AtomicInteger chosenAlterTransactionalTableAlternative, AtomicInteger chosenAlterPartitionsAlternative) throws TTransportException @@ -146,7 +137,6 @@ public ThriftHiveMetastoreClient( this.metastoreSupportsDateStatistics = requireNonNull(metastoreSupportsDateStatistics, "metastoreSupportsDateStatistics is null"); this.metastoreSupportsTableMeta = metastoreSupportsTableMeta; this.chosenGetTableAlternative = requireNonNull(chosenGetTableAlternative, "chosenGetTableAlternative is null"); - this.chosenTableParamAlternative = requireNonNull(chosenTableParamAlternative, "chosenTableParamAlternative is null"); this.chosenAlterTransactionalTableAlternative = requireNonNull(chosenAlterTransactionalTableAlternative, "chosenAlterTransactionalTableAlternative is null"); this.chosenAlterPartitionsAlternative = requireNonNull(chosenAlterPartitionsAlternative, "chosenAlterPartitionsAlternative is null"); this.catalogName = requireNonNull(catalogName, "catalogName is null"); @@ -220,41 +210,6 @@ public List getTableMeta(String databaseName) return client.getTableMeta(prependCatalogToDbName(catalogName, databaseName), "*", ImmutableList.of()); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues) - throws TException - { - checkArgument(TABLE_PARAMETER_SAFE_KEY_PATTERN.matcher(parameterKey).matches(), "Parameter key contains invalid characters: '%s'", parameterKey); - /* - * The parameter value is restricted to have only alphanumeric characters so that it's safe - * to be used against HMS. When using with a LIKE operator, the HMS may want the parameter - * value to follow a Java regex pattern or an SQL pattern. And it's hard to predict the - * HMS's behavior from outside. Also, by restricting parameter values, we avoid the problem - * of how to quote them when passing within the filter string. - */ - for (String parameterValue : parameterValues) { - checkArgument(TABLE_PARAMETER_SAFE_VALUE_PATTERN.matcher(parameterValue).matches(), "Parameter value contains invalid characters: '%s'", parameterValue); - } - /* - * Thrift call `get_table_names_by_filter` may be translated by Metastore to an SQL query against Metastore database. - * Hive 2.3 on some databases uses CLOB for table parameter value column and some databases disallow `=` predicate over - * CLOB values. At the same time, they allow `LIKE` predicates over them. - */ - String filterWithEquals = parameterValues.stream() - .map(parameterValue -> HIVE_FILTER_FIELD_PARAMS + parameterKey + " = \"" + parameterValue + "\"") - .collect(joining(" or ")); - - String filterWithLike = parameterValues.stream() - .map(parameterValue -> HIVE_FILTER_FIELD_PARAMS + parameterKey + " LIKE \"" + parameterValue + "\"") - .collect(joining(" or ")); - - return alternativeCall( - ThriftHiveMetastoreClient::defaultIsValidExceptionalResponse, - chosenTableParamAlternative, - () -> client.getTableNamesByFilter(databaseName, filterWithEquals, (short) -1), - () -> client.getTableNamesByFilter(databaseName, filterWithLike, (short) -1)); - } - @Override public void createDatabase(Database database) throws TException diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastore.java index 6f9cf7a04869..ebcdbf5359d4 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastore.java @@ -65,8 +65,6 @@ public sealed interface ThriftMetastore List getTables(String databaseName); - List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues); - Optional getDatabase(String databaseName); void addPartitions(String databaseName, String tableName, List partitions); diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java index 7fe9ef73f04f..b9c39b54a333 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java @@ -37,7 +37,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Set; public interface ThriftMetastoreClient extends Closeable @@ -54,9 +53,6 @@ Database getDatabase(String databaseName) List getTableMeta(String databaseName) throws TException; - List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues) - throws TException; - void createDatabase(Database database) throws TException; diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveMetadataListing.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveMetadataListing.java index af734b91ec22..bb96fefa3cad 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveMetadataListing.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveMetadataListing.java @@ -15,7 +15,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import io.trino.metastore.Column; import io.trino.metastore.Database; import io.trino.metastore.HiveBucketProperty; @@ -245,12 +244,6 @@ public List getTables(String databaseName) .build(); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, ImmutableSet parameterValues) - { - throw new UnsupportedOperationException(); - } - @Override public Optional
getTable(String databaseName, String tableName) { diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java index 080f685864d4..5503de707e43 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java @@ -49,7 +49,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import static com.google.common.collect.ImmutableList.toImmutableList; @@ -165,12 +164,6 @@ public List getTableMeta(String databaseName) return ImmutableList.of(new TableMeta(TEST_DATABASE, TEST_TABLE, MANAGED_TABLE.name())); } - @Override - public List getTableNamesWithParameters(String databaseName, String parameterKey, Set parameterValues) - { - throw new UnsupportedOperationException(); - } - @Override public Database getDatabase(String name) throws TException diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHiveMetastoreClient.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHiveMetastoreClient.java index 6faa578b65ab..c469374be025 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHiveMetastoreClient.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHiveMetastoreClient.java @@ -50,7 +50,6 @@ public void testAlternativeCall() true, new AtomicInteger(), new AtomicInteger(), - new AtomicInteger(), new AtomicInteger()); assertThat(connectionCount.get()).isEqualTo(1); From 72d087eeb7280ff965cfef27d9655b30eb916980 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 4/6] Revert "Extend testListTables with other relation types" This reverts commit 563ba351da6340109ace5c4c8624fa2355107b32. --- .../iceberg/catalog/BaseTrinoCatalogTest.java | 90 +------------------ ...TestTrinoHiveCatalogWithFileMetastore.java | 9 -- ...TestTrinoHiveCatalogWithHiveMetastore.java | 54 +---------- 3 files changed, 3 insertions(+), 150 deletions(-) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java index 1cf15d256401..4f8b355798da 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java @@ -21,12 +21,10 @@ import io.trino.plugin.base.util.AutoCloseableCloser; import io.trino.plugin.hive.NodeVersion; import io.trino.plugin.iceberg.CommitTaskData; -import io.trino.plugin.iceberg.IcebergFileFormat; import io.trino.plugin.iceberg.IcebergMetadata; import io.trino.plugin.iceberg.TableStatisticsWriter; import io.trino.spi.TrinoException; import io.trino.spi.connector.CatalogHandle; -import io.trino.spi.connector.ConnectorMaterializedViewDefinition; import io.trino.spi.connector.ConnectorMetadata; import io.trino.spi.connector.ConnectorSession; import io.trino.spi.connector.ConnectorViewDefinition; @@ -46,7 +44,6 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.time.Duration; import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -56,15 +53,10 @@ import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; import static io.airlift.json.JsonCodec.jsonCodec; import static io.trino.metastore.TableInfo.ExtendedRelationType.TABLE; -import static io.trino.metastore.TableInfo.ExtendedRelationType.TRINO_MATERIALIZED_VIEW; import static io.trino.metastore.TableInfo.ExtendedRelationType.TRINO_VIEW; import static io.trino.plugin.hive.HiveErrorCode.HIVE_DATABASE_LOCATION_ERROR; import static io.trino.plugin.iceberg.IcebergSchemaProperties.LOCATION_PROPERTY; -import static io.trino.plugin.iceberg.IcebergTableProperties.FILE_FORMAT_PROPERTY; -import static io.trino.plugin.iceberg.IcebergTableProperties.FORMAT_VERSION_PROPERTY; import static io.trino.plugin.iceberg.IcebergUtil.quotedTableName; -import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; -import static io.trino.spi.type.BigintType.BIGINT; import static io.trino.sql.planner.TestingPlannerContext.PLANNER_CONTEXT; import static io.trino.testing.TestingConnectorSession.SESSION; import static io.trino.testing.TestingNames.randomNameSuffix; @@ -446,60 +438,8 @@ public void testListTables() .commitTransaction(); closer.register(() -> catalog.dropTable(SESSION, table2)); - ImmutableList.Builder allTables = ImmutableList.builder() - .add(new TableInfo(table1, TABLE)) - .add(new TableInfo(table2, TABLE)); - - SchemaTableName view = new SchemaTableName(ns2, "view"); - try { - catalog.createView( - SESSION, - view, - new ConnectorViewDefinition( - "SELECT name FROM local.tiny.nation", - Optional.empty(), - Optional.empty(), - ImmutableList.of( - new ConnectorViewDefinition.ViewColumn("name", VarcharType.createUnboundedVarcharType().getTypeId(), Optional.empty())), - Optional.empty(), - Optional.of(SESSION.getUser()), - false, - ImmutableList.of()), - false); - closer.register(() -> catalog.dropView(SESSION, view)); - allTables.add(new TableInfo(view, getViewType())); - } - catch (TrinoException e) { - assertThat(e.getErrorCode()).isEqualTo(NOT_SUPPORTED.toErrorCode()); - } - - try { - SchemaTableName materializedView = new SchemaTableName(ns2, "mv"); - catalog.createMaterializedView( - SESSION, - materializedView, - someMaterializedView(), - ImmutableMap.of( - FILE_FORMAT_PROPERTY, IcebergFileFormat.PARQUET, - FORMAT_VERSION_PROPERTY, 1), - false, - false); - closer.register(() -> catalog.dropMaterializedView(SESSION, materializedView)); - allTables.add(new TableInfo(materializedView, TRINO_MATERIALIZED_VIEW)); - } - catch (TrinoException e) { - assertThat(e.getErrorCode()).isEqualTo(NOT_SUPPORTED.toErrorCode()); - } - - createExternalIcebergTable(catalog, ns2, closer).ifPresent(table -> { - allTables.add(new TableInfo(table, TABLE)); - }); - createExternalNonIcebergTable(catalog, ns2, closer).ifPresent(table -> { - allTables.add(new TableInfo(table, TABLE)); - }); - // No namespace provided, all tables across all namespaces should be returned - assertThat(catalog.listTables(SESSION, Optional.empty())).containsAll(allTables.build()); + assertThat(catalog.listTables(SESSION, Optional.empty())).containsAll(ImmutableList.of(new TableInfo(table1, TABLE), new TableInfo(table2, TABLE))); // Namespace is provided and exists assertThat(catalog.listTables(SESSION, Optional.of(ns1))).containsExactly(new TableInfo(table1, TABLE)); // Namespace is provided and does not exist @@ -507,18 +447,6 @@ public void testListTables() } } - protected Optional createExternalIcebergTable(TrinoCatalog catalog, String namespace, AutoCloseableCloser closer) - throws Exception - { - return Optional.empty(); - } - - protected Optional createExternalNonIcebergTable(TrinoCatalog catalog, String namespace, AutoCloseableCloser closer) - throws Exception - { - return Optional.empty(); - } - protected void assertViewDefinition(ConnectorViewDefinition actualView, ConnectorViewDefinition expectedView) { assertThat(actualView.getOriginalSql()).isEqualTo(expectedView.getOriginalSql()); @@ -532,7 +460,7 @@ protected void assertViewDefinition(ConnectorViewDefinition actualView, Connecto assertThat(actualView.isRunAsInvoker()).isEqualTo(expectedView.isRunAsInvoker()); } - protected String arbitraryTableLocation(TrinoCatalog catalog, ConnectorSession session, SchemaTableName schemaTableName) + private String arbitraryTableLocation(TrinoCatalog catalog, ConnectorSession session, SchemaTableName schemaTableName) throws Exception { try { @@ -553,18 +481,4 @@ private void assertViewColumnDefinition(ConnectorViewDefinition.ViewColumn actua assertThat(actualViewColumn.getName()).isEqualTo(expectedViewColumn.getName()); assertThat(actualViewColumn.getType()).isEqualTo(expectedViewColumn.getType()); } - - private static ConnectorMaterializedViewDefinition someMaterializedView() - { - return new ConnectorMaterializedViewDefinition( - "select 1", - Optional.empty(), - Optional.empty(), - Optional.empty(), - ImmutableList.of(new ConnectorMaterializedViewDefinition.Column("test", BIGINT.getTypeId(), Optional.empty())), - Optional.of(Duration.ZERO), - Optional.empty(), - Optional.of("owner"), - ImmutableList.of()); - } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java index c8641e0773f0..28c7beb01b61 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java @@ -56,7 +56,6 @@ import static io.trino.spi.type.IntegerType.INTEGER; import static io.trino.testing.TestingConnectorSession.SESSION; import static io.trino.testing.TestingNames.randomNameSuffix; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; @@ -155,12 +154,4 @@ private void testDropMaterializedView(boolean useUniqueTableLocations) } } } - - @Test - @Override - public void testListTables() - { - // the test actually works but when cleanup up the materialized view the error is thrown - assertThatThrownBy(super::testListTables).hasMessageMatching("Table 'ns2.*.mv' not found"); - } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java index c0f80a947eeb..01f4b8a4c3ed 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/hms/TestTrinoHiveCatalogWithHiveMetastore.java @@ -30,7 +30,6 @@ import io.trino.hdfs.authentication.NoHdfsAuthentication; import io.trino.hdfs.s3.HiveS3Config; import io.trino.hdfs.s3.TrinoS3ConfigurationInitializer; -import io.trino.metastore.Table; import io.trino.metastore.TableInfo; import io.trino.plugin.base.util.AutoCloseableCloser; import io.trino.plugin.hive.TrinoViewHiveMetastore; @@ -52,10 +51,6 @@ import io.trino.spi.security.PrincipalType; import io.trino.spi.security.TrinoPrincipal; import io.trino.spi.type.TestingTypeManager; -import org.apache.iceberg.PartitionSpec; -import org.apache.iceberg.Schema; -import org.apache.iceberg.SortOrder; -import org.apache.iceberg.types.Types; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -70,7 +65,6 @@ import static com.google.common.base.Verify.verify; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; -import static io.trino.metastore.PrincipalPrivileges.NO_PRIVILEGES; import static io.trino.plugin.hive.TestingThriftHiveMetastoreBuilder.testingThriftHiveMetastoreBuilder; import static io.trino.plugin.hive.containers.HiveHadoop.HIVE3_IMAGE; import static io.trino.plugin.hive.metastore.cache.CachingHiveMetastore.createPerTransactionCache; @@ -82,10 +76,7 @@ import static io.trino.testing.TestingNames.randomNameSuffix; import static io.trino.testing.containers.Minio.MINIO_ACCESS_KEY; import static io.trino.testing.containers.Minio.MINIO_SECRET_KEY; -import static java.util.Locale.ENGLISH; import static java.util.concurrent.TimeUnit.MINUTES; -import static org.apache.iceberg.BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE; -import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; @@ -101,7 +92,6 @@ public class TestTrinoHiveCatalogWithHiveMetastore // Use MinIO for storage, since HDFS is hard to get working in a unit test private HiveMinioDataLake dataLake; private TrinoFileSystem fileSystem; - private CachingHiveMetastore metastore; protected String bucketName; HiveMinioDataLake hiveMinioDataLake() @@ -148,7 +138,7 @@ protected TrinoCatalog createTrinoCatalog(boolean useUniqueTableLocations) .setReadTimeout(new Duration(1, MINUTES))) .metastoreClient(dataLake.getHiveMetastoreEndpoint()) .build(closer::register); - metastore = createPerTransactionCache(new BridgingHiveMetastore(thriftMetastore), 1000); + CachingHiveMetastore metastore = createPerTransactionCache(new BridgingHiveMetastore(thriftMetastore), 1000); fileSystem = fileSystemFactory.create(SESSION); return new TrinoHiveCatalog( @@ -245,48 +235,6 @@ public void testCreateMaterializedView() } } - @Override - protected Optional createExternalIcebergTable(TrinoCatalog catalog, String namespace, AutoCloseableCloser closer) - throws Exception - { - // simulate iceberg table created by spark with lowercase table type - return createTableWithTableType(catalog, namespace, closer, "lowercase_type", Optional.of(ICEBERG_TABLE_TYPE_VALUE.toLowerCase(ENGLISH))); - } - - @Override - protected Optional createExternalNonIcebergTable(TrinoCatalog catalog, String namespace, AutoCloseableCloser closer) - throws Exception - { - return createTableWithTableType(catalog, namespace, closer, "non_iceberg_table", Optional.empty()); - } - - private Optional createTableWithTableType(TrinoCatalog catalog, String namespace, AutoCloseableCloser closer, String tableName, Optional tableType) - throws Exception - { - SchemaTableName lowerCaseTableTypeTable = new SchemaTableName(namespace, tableName); - catalog.newCreateTableTransaction( - SESSION, - lowerCaseTableTypeTable, - new Schema(Types.NestedField.of(1, true, "col1", Types.LongType.get())), - PartitionSpec.unpartitioned(), - SortOrder.unsorted(), - arbitraryTableLocation(catalog, SESSION, lowerCaseTableTypeTable), - ImmutableMap.of()) - .commitTransaction(); - - Table metastoreTable = metastore.getTable(namespace, tableName).get(); - - metastore.replaceTable( - namespace, - tableName, - Table.builder(metastoreTable) - .setParameter(TABLE_TYPE_PROP, tableType) - .build(), - NO_PRIVILEGES); - closer.register(() -> metastore.dropTable(namespace, tableName, true)); - return Optional.of(lowerCaseTableTypeTable); - } - @Override protected Map defaultNamespaceProperties(String namespaceName) { From 0005004c7719029e6ce5931acf9639ab7735e489 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 5/6] Revert "Replace testView method override with getViewType" This reverts commit 8dfa5ab1acb4f9f3553b370786e183dd59cd9b3a. --- .../iceberg/catalog/BaseTrinoCatalogTest.java | 8 +- .../catalog/rest/TestTrinoRestCatalog.java | 73 +++++++++++++++++-- 2 files changed, 68 insertions(+), 13 deletions(-) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java index 4f8b355798da..a8aa117f9cde 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/BaseTrinoCatalogTest.java @@ -17,7 +17,6 @@ import com.google.common.collect.ImmutableMap; import io.airlift.log.Logger; import io.trino.metastore.TableInfo; -import io.trino.metastore.TableInfo.ExtendedRelationType; import io.trino.plugin.base.util.AutoCloseableCloser; import io.trino.plugin.hive.NodeVersion; import io.trino.plugin.iceberg.CommitTaskData; @@ -365,7 +364,7 @@ public void testView() catalog.createNamespace(SESSION, namespace, defaultNamespaceProperties(namespace), new TrinoPrincipal(PrincipalType.USER, SESSION.getUser())); catalog.createView(SESSION, schemaTableName, viewDefinition, false); - assertThat(catalog.listTables(SESSION, Optional.of(namespace)).stream()).contains(new TableInfo(schemaTableName, getViewType())); + assertThat(catalog.listTables(SESSION, Optional.of(namespace)).stream()).contains(new TableInfo(schemaTableName, TRINO_VIEW)); Map views = catalog.getViews(SESSION, Optional.of(schemaTableName.getSchemaName())); assertThat(views).hasSize(1); @@ -394,11 +393,6 @@ public void testView() } } - protected ExtendedRelationType getViewType() - { - return TRINO_VIEW; - } - @Test public void testListTables() throws Exception diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java index 398ee371ad1b..df3ad2a5400c 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java @@ -13,7 +13,9 @@ */ package io.trino.plugin.iceberg.catalog.rest; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import io.airlift.log.Logger; import io.trino.cache.EvictableCacheBuilder; import io.trino.metastore.TableInfo; import io.trino.plugin.hive.NodeVersion; @@ -25,9 +27,12 @@ import io.trino.spi.catalog.CatalogName; import io.trino.spi.connector.CatalogHandle; import io.trino.spi.connector.ConnectorMetadata; +import io.trino.spi.connector.ConnectorViewDefinition; +import io.trino.spi.connector.SchemaTableName; import io.trino.spi.security.PrincipalType; import io.trino.spi.security.TrinoPrincipal; import io.trino.spi.type.TestingTypeManager; +import io.trino.spi.type.VarcharType; import org.apache.iceberg.exceptions.BadRequestException; import org.apache.iceberg.rest.DelegatingRestSessionCatalog; import org.apache.iceberg.rest.RESTSessionCatalog; @@ -35,6 +40,8 @@ import org.junit.jupiter.api.Test; import java.io.File; +import java.io.IOException; +import java.nio.file.Path; import java.util.Map; import java.util.Optional; @@ -55,6 +62,8 @@ public class TestTrinoRestCatalog extends BaseTrinoCatalogTest { + private static final Logger LOG = Logger.get(TestTrinoRestCatalog.class); + @Override protected TrinoCatalog createTrinoCatalog(boolean useUniqueTableLocations) { @@ -136,6 +145,64 @@ public void testNonLowercaseNamespace() } } + @Test + @Override + public void testView() + throws IOException + { + TrinoCatalog catalog = createTrinoCatalog(false); + Path tmpDirectory = java.nio.file.Files.createTempDirectory("iceberg_catalog_test_create_view_"); + tmpDirectory.toFile().deleteOnExit(); + + String namespace = "test_create_view_" + randomNameSuffix(); + String viewName = "viewName"; + String renamedViewName = "renamedViewName"; + SchemaTableName schemaTableName = new SchemaTableName(namespace, viewName); + SchemaTableName renamedSchemaTableName = new SchemaTableName(namespace, renamedViewName); + ConnectorViewDefinition viewDefinition = new ConnectorViewDefinition( + "SELECT name FROM local.tiny.nation", + Optional.empty(), + Optional.empty(), + ImmutableList.of( + new ConnectorViewDefinition.ViewColumn("name", VarcharType.createUnboundedVarcharType().getTypeId(), Optional.empty())), + Optional.empty(), + Optional.of(SESSION.getUser()), + false, + ImmutableList.of()); + + try { + catalog.createNamespace(SESSION, namespace, ImmutableMap.of(), new TrinoPrincipal(PrincipalType.USER, SESSION.getUser())); + catalog.createView(SESSION, schemaTableName, viewDefinition, false); + + assertThat(catalog.listTables(SESSION, Optional.of(namespace)).stream()).contains(new TableInfo(schemaTableName, OTHER_VIEW)); + + Map views = catalog.getViews(SESSION, Optional.of(schemaTableName.getSchemaName())); + assertThat(views).hasSize(1); + assertViewDefinition(views.get(schemaTableName), viewDefinition); + assertViewDefinition(catalog.getView(SESSION, schemaTableName).orElseThrow(), viewDefinition); + + catalog.renameView(SESSION, schemaTableName, renamedSchemaTableName); + assertThat(catalog.listTables(SESSION, Optional.of(namespace)).stream().map(TableInfo::tableName).toList()).doesNotContain(schemaTableName); + views = catalog.getViews(SESSION, Optional.of(schemaTableName.getSchemaName())); + assertThat(views).hasSize(1); + assertViewDefinition(views.get(renamedSchemaTableName), viewDefinition); + assertViewDefinition(catalog.getView(SESSION, renamedSchemaTableName).orElseThrow(), viewDefinition); + assertThat(catalog.getView(SESSION, schemaTableName)).isEmpty(); + + catalog.dropView(SESSION, renamedSchemaTableName); + assertThat(catalog.listTables(SESSION, Optional.empty()).stream().map(TableInfo::tableName).toList()) + .doesNotContain(renamedSchemaTableName); + } + finally { + try { + catalog.dropNamespace(SESSION, namespace); + } + catch (Exception e) { + LOG.warn("Failed to clean up namespace: %s", namespace); + } + } + } + @Test public void testPrefix() { @@ -153,10 +220,4 @@ public void testPrefix() .as("should fail as the prefix dev is not implemented for the current endpoint") .hasMessageContaining("Malformed request: No route for request: POST v1/dev/namespaces"); } - - @Override - protected TableInfo.ExtendedRelationType getViewType() - { - return OTHER_VIEW; - } } From e7692c03a71e3734e4aa69660a683912b07d517d Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 23 Dec 2024 23:01:54 +0900 Subject: [PATCH 6/6] Revert "Add tests for dropMaterializedView" This reverts commit 21306cd6a57f1d1cd81bf7447e79581325a3785e. --- ...TestTrinoHiveCatalogWithFileMetastore.java | 67 ------------------- 1 file changed, 67 deletions(-) diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java index 28c7beb01b61..a23952cc3167 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java @@ -13,9 +13,6 @@ */ package io.trino.plugin.iceberg.catalog.file; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import io.airlift.log.Logger; import io.trino.filesystem.Location; import io.trino.filesystem.TrinoFileSystemFactory; import io.trino.filesystem.local.LocalFileSystemFactory; @@ -27,15 +24,9 @@ import io.trino.plugin.iceberg.catalog.TrinoCatalog; import io.trino.plugin.iceberg.catalog.hms.TrinoHiveCatalog; import io.trino.spi.catalog.CatalogName; -import io.trino.spi.connector.ConnectorMaterializedViewDefinition; -import io.trino.spi.connector.SchemaTableName; -import io.trino.spi.security.PrincipalType; -import io.trino.spi.security.TrinoPrincipal; import io.trino.spi.type.TestingTypeManager; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.parallel.Execution; @@ -43,19 +34,12 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.util.Optional; 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.directExecutor; import static io.trino.plugin.hive.metastore.cache.CachingHiveMetastore.createPerTransactionCache; import static io.trino.plugin.hive.metastore.file.TestingFileHiveMetastore.createTestingFileHiveMetastore; -import static io.trino.plugin.iceberg.IcebergFileFormat.PARQUET; -import static io.trino.plugin.iceberg.IcebergTableProperties.FILE_FORMAT_PROPERTY; -import static io.trino.plugin.iceberg.IcebergTableProperties.FORMAT_VERSION_PROPERTY; -import static io.trino.spi.type.IntegerType.INTEGER; -import static io.trino.testing.TestingConnectorSession.SESSION; -import static io.trino.testing.TestingNames.randomNameSuffix; import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; @@ -64,8 +48,6 @@ public class TestTrinoHiveCatalogWithFileMetastore extends BaseTrinoCatalogTest { - private static final Logger log = Logger.get(TestTrinoHiveCatalogWithFileMetastore.class); - private Path tempDir; private TrinoFileSystemFactory fileSystemFactory; private HiveMetastore metastore; @@ -105,53 +87,4 @@ protected TrinoCatalog createTrinoCatalog(boolean useUniqueTableLocations) new IcebergConfig().isHideMaterializedViewStorageTable(), directExecutor()); } - - @Test - @Disabled - public void testDropMaterializedView() - { - testDropMaterializedView(false); - } - - @Test - public void testDropMaterializedViewWithUniqueTableLocation() - { - testDropMaterializedView(true); - } - - private void testDropMaterializedView(boolean useUniqueTableLocations) - { - TrinoCatalog catalog = createTrinoCatalog(useUniqueTableLocations); - String namespace = "test_create_mv_" + randomNameSuffix(); - String materializedViewName = "materialized_view_name"; - try { - catalog.createNamespace(SESSION, namespace, defaultNamespaceProperties(namespace), new TrinoPrincipal(PrincipalType.USER, SESSION.getUser())); - catalog.createMaterializedView( - SESSION, - new SchemaTableName(namespace, materializedViewName), - new ConnectorMaterializedViewDefinition( - "SELECT * FROM tpch.tiny.nation", - Optional.empty(), - Optional.of("catalog_name"), - Optional.of("schema_name"), - ImmutableList.of(new ConnectorMaterializedViewDefinition.Column("col1", INTEGER.getTypeId(), Optional.empty())), - Optional.empty(), - Optional.empty(), - Optional.empty(), - ImmutableList.of()), - ImmutableMap.of(FILE_FORMAT_PROPERTY, PARQUET, FORMAT_VERSION_PROPERTY, 1), - false, - false); - - catalog.dropMaterializedView(SESSION, new SchemaTableName(namespace, materializedViewName)); - } - finally { - try { - catalog.dropNamespace(SESSION, namespace); - } - catch (Exception e) { - log.warn("Failed to clean up namespace: %s", namespace); - } - } - } }