diff --git a/.mvn/modernizer/violations.xml b/.mvn/modernizer/violations.xml index 12045c09097b..c5ddee06db55 100644 --- a/.mvn/modernizer/violations.xml +++ b/.mvn/modernizer/violations.xml @@ -317,4 +317,10 @@ 1.8 Use io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder() instead + + + software/amazon/awssdk/services/glue/model/Table.tableType:()Ljava/lang/String; + 1.8 + Table type is nullable in Glue model, which is too easy to forget about. Prefer GlueConverter.getTableTypeNullable + diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueConverter.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueConverter.java index e36dacc6c04a..03785bde62f2 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueConverter.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueConverter.java @@ -40,6 +40,8 @@ import io.trino.spi.TrinoException; import io.trino.spi.function.LanguageFunction; import io.trino.spi.security.PrincipalType; +import jakarta.annotation.Nullable; +import org.gaul.modernizer_maven_annotations.SuppressModernizer; import software.amazon.awssdk.core.SdkBytes; import software.amazon.awssdk.services.glue.model.BinaryColumnStatisticsData; import software.amazon.awssdk.services.glue.model.BooleanColumnStatisticsData; @@ -88,6 +90,7 @@ import static io.trino.metastore.Table.TABLE_COMMENT; import static io.trino.plugin.hive.HiveErrorCode.HIVE_INVALID_METADATA; import static io.trino.plugin.hive.HiveErrorCode.HIVE_UNSUPPORTED_FORMAT; +import static io.trino.plugin.hive.TableType.EXTERNAL_TABLE; import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView; import static io.trino.plugin.hive.ViewReaderUtil.isTrinoView; import static io.trino.plugin.hive.metastore.MetastoreUtil.metastoreFunctionName; @@ -117,6 +120,19 @@ final class GlueConverter private GlueConverter() {} + public static String getTableType(software.amazon.awssdk.services.glue.model.Table glueTable) + { + // Athena treats a missing table type as EXTERNAL_TABLE. + return firstNonNull(getTableTypeNullable(glueTable), EXTERNAL_TABLE.name()); + } + + @Nullable + @SuppressModernizer // Usage of `Table.tableType` is not allowed. Only this method can call that. + public static String getTableTypeNullable(software.amazon.awssdk.services.glue.model.Table glueTable) + { + return glueTable.tableType(); + } + public static Database fromGlueDatabase(software.amazon.awssdk.services.glue.model.Database glueDb) { return new Database( @@ -140,8 +156,7 @@ public static DatabaseInput toGlueDatabaseInput(Database database) public static Table fromGlueTable(software.amazon.awssdk.services.glue.model.Table glueTable, String databaseName) { - // Athena treats a missing table type as EXTERNAL_TABLE. - String tableType = firstNonNull(glueTable.tableType(), "EXTERNAL_TABLE"); + String tableType = getTableType(glueTable); Map tableParameters = glueTable.parameters(); if (glueTable.description() != null) { 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 a7103e4326cc..6d428eaac57f 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 @@ -122,6 +122,7 @@ import static io.trino.plugin.hive.metastore.MetastoreUtil.toPartitionName; import static io.trino.plugin.hive.metastore.MetastoreUtil.updateStatisticsParameters; import static io.trino.plugin.hive.metastore.glue.GlueConverter.fromGlueStatistics; +import static io.trino.plugin.hive.metastore.glue.GlueConverter.getTableTypeNullable; import static io.trino.plugin.hive.metastore.glue.GlueConverter.toGlueColumnStatistics; import static io.trino.plugin.hive.metastore.glue.GlueConverter.toGlueDatabaseInput; import static io.trino.plugin.hive.metastore.glue.GlueConverter.toGlueFunctionInput; @@ -448,7 +449,7 @@ private List getTablesInternal(Consumer cacheTable, String dat return glueTables.stream() .map(table -> new TableInfo( new SchemaTableName(databaseName, table.name()), - TableInfo.ExtendedRelationType.fromTableTypeAndComment(table.tableType(), table.parameters().get(TABLE_COMMENT)))) + TableInfo.ExtendedRelationType.fromTableTypeAndComment(GlueConverter.getTableType(table), table.parameters().get(TABLE_COMMENT)))) .toList(); } catch (EntityNotFoundException _) { @@ -716,7 +717,7 @@ public static TableInput.Builder asTableInputBuilder(software.amazon.awssdk.serv .partitionKeys(table.partitionKeys()) .viewOriginalText(table.viewOriginalText()) .viewExpandedText(table.viewExpandedText()) - .tableType(table.tableType()) + .tableType(getTableTypeNullable(table)) .targetTable(table.targetTable()) .parameters(table.parameters()); } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java index 42fc5ff5f07d..b1c13d26b132 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java @@ -32,6 +32,8 @@ import software.amazon.awssdk.services.glue.model.TableInput; import java.nio.file.Path; +import java.util.List; +import java.util.Set; import static io.trino.plugin.hive.metastore.glue.GlueMetastoreModule.createGlueClient; import static io.trino.plugin.hive.metastore.glue.TestingGlueHiveMetastore.createTestingGlueHiveMetastore; @@ -46,6 +48,7 @@ public class TestHiveGlueMetadataListing extends AbstractTestQueryFramework { public static final String FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME = "failing_table_with_null_storage_descriptor"; + public static final String FAILING_TABLE_WITH_NULL_TYPE = "failing_table_with_null_type"; private static final Logger LOG = Logger.get(TestHiveGlueMetadataListing.class); private static final String HIVE_CATALOG = "hive"; private final String tpchSchema = "test_tpch_schema_" + randomNameSuffix(); @@ -75,7 +78,7 @@ protected QueryRunner createQueryRunner() queryRunner.execute("CREATE SCHEMA " + tpchSchema + " WITH (location = '" + dataDirectory.toUri() + "')"); copyTpchTables(queryRunner, "tpch", TINY_SCHEMA_NAME, hiveSession, ImmutableList.of(TpchTable.REGION, TpchTable.NATION)); - createBrokenTable(dataDirectory); + createBrokenTables(dataDirectory); return queryRunner; } @@ -98,43 +101,54 @@ public void cleanup() @Test public void testReadInformationSchema() { - String expectedTables = format("VALUES '%s', '%s', '%s'", TpchTable.REGION.getTableName(), TpchTable.NATION.getTableName(), FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME); - - assertThat(query("SELECT table_name FROM hive.information_schema.tables")) - .skippingTypesCheck() - .containsAll(expectedTables); - assertThat(query("SELECT table_name FROM hive.information_schema.tables WHERE table_schema='" + tpchSchema + "'")) - .skippingTypesCheck() - .matches(expectedTables); - assertThat(query("SELECT table_name FROM hive.information_schema.tables WHERE table_name = 'region' AND table_schema='" + tpchSchema + "'")) - .skippingTypesCheck() - .matches("VALUES 'region'"); + Set expectedTables = ImmutableSet.builder() + .add(TpchTable.REGION.getTableName()) + .add(TpchTable.NATION.getTableName()) + .add(FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME) + .add(FAILING_TABLE_WITH_NULL_TYPE) + .build(); + + assertThat(computeActual("SELECT table_name FROM hive.information_schema.tables").getOnlyColumnAsSet()).containsAll(expectedTables); + assertThat(computeActual("SELECT table_name FROM hive.information_schema.tables WHERE table_schema='" + tpchSchema + "'").getOnlyColumnAsSet()).containsAll(expectedTables); + assertThat(computeScalar("SELECT table_name FROM hive.information_schema.tables WHERE table_name = 'region' AND table_schema='" + tpchSchema + "'")) + .isEqualTo(TpchTable.REGION.getTableName()); assertQueryReturnsEmptyResult(format("SELECT table_name FROM hive.information_schema.tables WHERE table_name = '%s' AND table_schema='%s'", FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME, tpchSchema)); + assertQueryReturnsEmptyResult(format("SELECT table_name FROM hive.information_schema.tables WHERE table_name = '%s' AND table_schema='%s'", FAILING_TABLE_WITH_NULL_TYPE, tpchSchema)); assertQuery("SELECT table_name, column_name from hive.information_schema.columns WHERE table_schema = '" + tpchSchema + "'", "VALUES ('region', 'regionkey'), ('region', 'name'), ('region', 'comment'), ('nation', 'nationkey'), ('nation', 'name'), ('nation', 'regionkey'), ('nation', 'comment')"); assertQuery("SELECT table_name, column_name from hive.information_schema.columns WHERE table_name = 'region' AND table_schema='" + tpchSchema + "'", "VALUES ('region', 'regionkey'), ('region', 'name'), ('region', 'comment')"); assertQueryReturnsEmptyResult(format("SELECT table_name FROM hive.information_schema.columns WHERE table_name = '%s' AND table_schema='%s'", FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME, tpchSchema)); + assertQueryReturnsEmptyResult(format("SELECT table_name FROM hive.information_schema.columns WHERE table_name = '%s' AND table_schema='%s'", FAILING_TABLE_WITH_NULL_TYPE, tpchSchema)); + + assertThat(computeActual("SHOW TABLES FROM hive." + tpchSchema).getOnlyColumnAsSet()).isEqualTo(expectedTables); + } - assertQuery("SHOW TABLES FROM hive." + tpchSchema, expectedTables); + private void createBrokenTables(Path dataDirectory) + { + TableInput nullStorageTable = TableInput.builder() + .name(FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME) + .tableType("HIVE") + .build(); + TableInput nullTypeTable = TableInput.builder() + .name(FAILING_TABLE_WITH_NULL_TYPE) + .build(); + createBrokenTable(List.of(nullStorageTable, nullTypeTable), dataDirectory); } - private void createBrokenTable(Path dataDirectory) + private void createBrokenTable(List tablesInput, Path dataDirectory) { GlueHiveMetastoreConfig glueConfig = new GlueHiveMetastoreConfig() .setDefaultWarehouseDir(dataDirectory.toString()); try (GlueClient glueClient = createGlueClient(glueConfig, ImmutableSet.of())) { - TableInput tableInput = TableInput.builder() - .name(FAILING_TABLE_WITH_NULL_STORAGE_DESCRIPTOR_NAME) - .tableType("HIVE") - .build(); - - CreateTableRequest createTableRequest = CreateTableRequest.builder() - .databaseName(tpchSchema) - .tableInput(tableInput) - .build(); - glueClient.createTable(createTableRequest); + for (TableInput tableInput : tablesInput) { + CreateTableRequest createTableRequest = CreateTableRequest.builder() + .databaseName(tpchSchema) + .tableInput(tableInput) + .build(); + glueClient.createTable(createTableRequest); + } } } } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueConverter.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueConverter.java index 4eda8e552254..08e47220ba2f 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueConverter.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueConverter.java @@ -47,6 +47,7 @@ import static io.trino.plugin.hive.TableType.EXTERNAL_TABLE; import static io.trino.plugin.hive.ViewReaderUtil.PRESTO_VIEW_FLAG; import static io.trino.plugin.hive.metastore.glue.GlueConverter.PUBLIC_OWNER; +import static io.trino.plugin.hive.metastore.glue.GlueConverter.getTableTypeNullable; import static io.trino.plugin.hive.util.HiveUtil.DELTA_LAKE_PROVIDER; import static io.trino.plugin.hive.util.HiveUtil.ICEBERG_TABLE_TYPE_NAME; import static io.trino.plugin.hive.util.HiveUtil.ICEBERG_TABLE_TYPE_VALUE; @@ -246,7 +247,7 @@ void testConvertTable() io.trino.metastore.Table trinoTable = GlueConverter.fromGlueTable(glueTable, glueDatabase.name()); assertThat(trinoTable.getTableName()).isEqualTo(glueTable.name()); assertThat(trinoTable.getDatabaseName()).isEqualTo(glueDatabase.name()); - assertThat(trinoTable.getTableType()).isEqualTo(glueTable.tableType()); + assertThat(trinoTable.getTableType()).isEqualTo(getTableTypeNullable(glueTable)); assertThat(trinoTable.getOwner().orElse(null)).isEqualTo(glueTable.owner()); assertThat(trinoTable.getParameters()).isEqualTo(glueTable.parameters()); assertColumnList(glueTable.storageDescriptor().columns(), trinoTable.getDataColumns()); @@ -278,7 +279,7 @@ void testConvertTableWithOpenCSVSerDe() assertThat(trinoTable.getTableName()).isEqualTo(glueTable.name()); assertThat(trinoTable.getDatabaseName()).isEqualTo(glueDatabase.name()); - assertThat(trinoTable.getTableType()).isEqualTo(glueTable.tableType()); + assertThat(trinoTable.getTableType()).isEqualTo(getTableTypeNullable(glueTable)); assertThat(trinoTable.getOwner().orElse(null)).isEqualTo(glueTable.owner()); assertThat(trinoTable.getParameters()).isEqualTo(glueTable.parameters()); assertThat(trinoTable.getDataColumns()).hasSize(1);