Skip to content

Fix listing Glue tables with null type #24859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Fix listing Glue tables with null type #24859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
935ac70
Simplify HiveGlueMetadataListing test
pajaks Feb 3, 2025
db916a4
Prevent usage of glue.model.Table.tableType()
pajaks Feb 3, 2025
b3f72ec
Extract getTableType in GlueConverter
pajaks Feb 3, 2025
72126e2
Fix listing Glue tables with null type
pajaks Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .mvn/modernizer/violations.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,4 +317,10 @@
<version>1.8</version>
<comment>Use io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder() instead</comment>
</violation>

<violation>
<name>software/amazon/awssdk/services/glue/model/Table.tableType:()Ljava/lang/String;</name>
<version>1.8</version>
<comment>Table type is nullable in Glue model, which is too easy to forget about. Prefer GlueConverter.getTableTypeNullable</comment>
</violation>
</modernizer>
19 changes: 17 additions & 2 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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<String, String> tableParameters = glueTable.parameters();
if (glueTable.description() != null) {
Expand Down
5 changes: 3 additions & 2 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -448,7 +449,7 @@ private List<TableInfo> getTablesInternal(Consumer<Table> 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 _) {
Expand Down Expand Up @@ -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());
}
Expand Down
62 changes: 38 additions & 24 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveGlueMetadataListing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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;
}
Expand All @@ -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<String> expectedTables = ImmutableSet.<String>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<TableInput> 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);
}
}
}
}
5 changes: 3 additions & 2 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down