Skip to content

Fix NPE when Glue table has no type #15909

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
findepi merged 1 commit into from
Feb 1, 2023
Merged

Fix NPE when Glue table has no type #15909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 @@ -140,6 +140,12 @@
<comment>Use AssertJ's assertThatThrownBy, see https://github.com/trinodb/trino/issues/5320 for rationale</comment>
</violation>

<violation>
<name>com/amazonaws/services/glue/model/Table.getTableType:()Ljava/lang/String;</name>
<version>1.1</version>
<comment>Table type is nullable in Glue model, which is too easy to forget about. Prefer GlueToTrinoConverter.getTableType</comment>
</violation>

<violation>
<name>org/apache/hadoop/mapred/JobConf."&lt;init&gt;":()V</name>
<version>1.1</version>
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 @@ -151,6 +151,7 @@
import static io.trino.plugin.hive.metastore.MetastoreUtil.verifyCanDropColumn;
import static io.trino.plugin.hive.metastore.glue.AwsSdkUtil.getPaginatedResults;
import static io.trino.plugin.hive.metastore.glue.converter.GlueInputConverter.convertPartition;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableTypeNullable;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.mappedCopy;
import static io.trino.plugin.hive.metastore.thrift.ThriftMetastoreUtil.getHiveBasicStatistics;
import static io.trino.plugin.hive.metastore.thrift.ThriftMetastoreUtil.updateStatisticsParameters;
Expand All @@ -177,7 +178,7 @@ public class GlueHiveMetastore
private static final int BATCH_UPDATE_PARTITION_MAX_PAGE_SIZE = 100;
private static final int AWS_GLUE_GET_PARTITIONS_MAX_RESULTS = 1000;
private static final Comparator<Iterable<String>> PARTITION_VALUE_COMPARATOR = lexicographical(String.CASE_INSENSITIVE_ORDER);
private static final Predicate<com.amazonaws.services.glue.model.Table> VIEWS_FILTER = table -> VIRTUAL_VIEW.name().equals(table.getTableType());
private static final Predicate<com.amazonaws.services.glue.model.Table> VIEWS_FILTER = table -> VIRTUAL_VIEW.name().equals(getTableTypeNullable(table));

private final HdfsEnvironment hdfsEnvironment;
private final HdfsContext hdfsContext;
Expand Down Expand Up @@ -703,7 +704,7 @@ private TableInput convertGlueTableToTableInput(com.amazonaws.services.glue.mode
.withPartitionKeys(glueTable.getPartitionKeys())
.withViewOriginalText(glueTable.getViewOriginalText())
.withViewExpandedText(glueTable.getViewExpandedText())
.withTableType(glueTable.getTableType())
.withTableType(getTableTypeNullable(glueTable))
.withTargetTable(glueTable.getTargetTable())
.withParameters(glueTable.getParameters());
}
Expand Down
19 changes: 17 additions & 2 deletions ...ive/src/main/java/io/trino/plugin/hive/metastore/glue/converter/GlueToTrinoConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
import io.trino.plugin.hive.util.HiveBucketing.BucketingVersion;
import io.trino.spi.TrinoException;
import io.trino.spi.security.PrincipalType;
import org.gaul.modernizer_maven_annotations.SuppressModernizer;

import javax.annotation.Nullable;

import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -59,6 +62,19 @@ public final class GlueToTrinoConverter

private GlueToTrinoConverter() {}

public static String getTableType(com.amazonaws.services.glue.model.Table glueTable)
{
// Athena treats missing table type as EXTERNAL_TABLE.
return firstNonNull(getTableTypeNullable(glueTable), EXTERNAL_TABLE.name());
}

@Nullable
@SuppressModernizer // Usage of `Table.getTableType` is not allowed. Only this method can call that.
public static String getTableTypeNullable(com.amazonaws.services.glue.model.Table glueTable)
{
return glueTable.getTableType();
}

public static Database convertDatabase(com.amazonaws.services.glue.model.Database glueDb)
{
return Database.builder()
Expand All @@ -81,8 +97,7 @@ public static Table convertTable(com.amazonaws.services.glue.model.Table glueTab
.setDatabaseName(dbName)
.setTableName(glueTable.getName())
.setOwner(Optional.ofNullable(glueTable.getOwner()))
// Athena treats missing table type as EXTERNAL_TABLE.
.setTableType(firstNonNull(glueTable.getTableType(), EXTERNAL_TABLE.name()))
.setTableType(getTableType(glueTable))
.setParameters(tableParameters)
.setViewOriginalText(Optional.ofNullable(glueTable.getViewOriginalText()))
.setViewExpandedText(Optional.ofNullable(glueTable.getViewExpandedText()));
Expand Down
5 changes: 3 additions & 2 deletions ...rino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueToTrinoConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import static io.trino.plugin.hive.metastore.glue.TestingMetastoreObjects.getGlueTestPartition;
import static io.trino.plugin.hive.metastore.glue.TestingMetastoreObjects.getGlueTestStorageDescriptor;
import static io.trino.plugin.hive.metastore.glue.TestingMetastoreObjects.getGlueTestTable;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.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 @@ -93,7 +94,7 @@ public void testConvertTable()
io.trino.plugin.hive.metastore.Table trinoTable = GlueToTrinoConverter.convertTable(testTable, testDatabase.getName());
assertEquals(trinoTable.getTableName(), testTable.getName());
assertEquals(trinoTable.getDatabaseName(), testDatabase.getName());
assertEquals(trinoTable.getTableType(), testTable.getTableType());
assertEquals(trinoTable.getTableType(), getTableTypeNullable(testTable));
assertEquals(trinoTable.getOwner().orElse(null), testTable.getOwner());
assertEquals(trinoTable.getParameters(), testTable.getParameters());
assertColumnList(trinoTable.getDataColumns(), testTable.getStorageDescriptor().getColumns());
Expand All @@ -114,7 +115,7 @@ public void testConvertTableWithOpenCSVSerDe()

assertEquals(trinoTable.getTableName(), glueTable.getName());
assertEquals(trinoTable.getDatabaseName(), testDatabase.getName());
assertEquals(trinoTable.getTableType(), glueTable.getTableType());
assertEquals(trinoTable.getTableType(), getTableTypeNullable(glueTable));
assertEquals(trinoTable.getOwner().orElse(null), glueTable.getOwner());
assertEquals(trinoTable.getParameters(), glueTable.getParameters());
assertEquals(trinoTable.getDataColumns().size(), 1);
Expand Down
3 changes: 2 additions & 1 deletion ...ceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import static com.google.common.base.Verify.verify;
import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableType;
import static io.trino.plugin.hive.util.HiveUtil.isIcebergTable;
import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_INVALID_METADATA;
import static io.trino.plugin.iceberg.catalog.glue.GlueIcebergUtil.getTableInput;
Expand Down Expand Up @@ -83,7 +84,7 @@ protected String getRefreshedLocation(boolean invalidateCaches)
glueVersionId = table.getVersionId();

Map<String, String> parameters = firstNonNull(table.getParameters(), ImmutableMap.of());
if (isPrestoView(parameters) && isHiveOrPrestoView(table.getTableType())) {
if (isPrestoView(parameters) && isHiveOrPrestoView(getTableType(table))) {
// this is a Presto Hive view, hence not a table
throw new TableNotFoundException(getSchemaTableName());
}
Expand Down
20 changes: 11 additions & 9 deletions ...in/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView;
import static io.trino.plugin.hive.metastore.glue.AwsSdkUtil.getPaginatedResults;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableType;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableTypeNullable;
import static io.trino.plugin.hive.util.HiveUtil.isHiveSystemSchema;
import static io.trino.plugin.hive.util.HiveUtil.isIcebergTable;
import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_BAD_DATA;
Expand Down Expand Up @@ -464,7 +466,7 @@ private Optional<com.amazonaws.services.glue.model.Table> getTable(ConnectorSess
LOG.warn(e, "Failed to cache table metadata from table at %s", metadataLocation);
}
}
else if (isTrinoMaterializedView(table.getTableType(), parameters)) {
else if (isTrinoMaterializedView(getTableType(table), parameters)) {
if (viewCache.containsKey(schemaTableName) || tableMetadataCache.containsKey(schemaTableName)) {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Glue table cache inconsistency. Materialized View cannot also be a table or view");
}
Expand All @@ -485,7 +487,7 @@ else if (isPrestoView(parameters) && !viewCache.containsKey(schemaTableName)) {
try {
TrinoViewUtil.getView(schemaTableName,
Optional.ofNullable(table.getViewOriginalText()),
table.getTableType(),
getTableType(table),
parameters,
Optional.ofNullable(table.getOwner()))
.ifPresent(viewDefinition -> viewCache.put(schemaTableName, viewDefinition));
Expand Down Expand Up @@ -708,7 +710,7 @@ public Optional<ConnectorViewDefinition> getView(ConnectorSession session, Schem
return TrinoViewUtil.getView(
viewName,
Optional.ofNullable(viewDefinition.getViewOriginalText()),
viewDefinition.getTableType(),
getTableType(viewDefinition),
firstNonNull(viewDefinition.getParameters(), ImmutableMap.of()),
Optional.ofNullable(viewDefinition.getOwner()));
}
Expand Down Expand Up @@ -784,7 +786,7 @@ public List<SchemaTableName> listMaterializedViews(ConnectorSession session, Opt
stats.getGetTables())
.map(GetTablesResult::getTableList)
.flatMap(List::stream)
.filter(table -> isTrinoMaterializedView(table.getTableType(), firstNonNull(table.getParameters(), ImmutableMap.of())))
.filter(table -> isTrinoMaterializedView(getTableType(table), firstNonNull(table.getParameters(), ImmutableMap.of())))
.map(table -> new SchemaTableName(glueNamespace, table.getName()));
}
catch (EntityNotFoundException e) {
Expand All @@ -810,7 +812,7 @@ public void createMaterializedView(
Optional<com.amazonaws.services.glue.model.Table> existing = getTable(session, viewName);

if (existing.isPresent()) {
if (!isTrinoMaterializedView(existing.get().getTableType(), firstNonNull(existing.get().getParameters(), ImmutableMap.of()))) {
if (!isTrinoMaterializedView(getTableType(existing.get()), firstNonNull(existing.get().getParameters(), ImmutableMap.of()))) {
throw new TrinoException(UNSUPPORTED_TABLE_TYPE, "Existing table is not a Materialized View: " + viewName);
}
if (!replace) {
Expand Down Expand Up @@ -862,7 +864,7 @@ public void dropMaterializedView(ConnectorSession session, SchemaTableName viewN
com.amazonaws.services.glue.model.Table view = getTable(session, viewName)
.orElseThrow(() -> new MaterializedViewNotFoundException(viewName));

if (!isTrinoMaterializedView(view.getTableType(), firstNonNull(view.getParameters(), ImmutableMap.of()))) {
if (!isTrinoMaterializedView(getTableType(view), firstNonNull(view.getParameters(), ImmutableMap.of()))) {
throw new TrinoException(UNSUPPORTED_TABLE_TYPE, "Not a Materialized View: " + view.getDatabaseName() + "." + view.getName());
}
materializedViewCache.remove(viewName);
Expand Down Expand Up @@ -905,7 +907,7 @@ protected Optional<ConnectorMaterializedViewDefinition> doGetMaterializedView(Co
}

com.amazonaws.services.glue.model.Table table = maybeTable.get();
if (!isTrinoMaterializedView(table.getTableType(), firstNonNull(table.getParameters(), ImmutableMap.of()))) {
if (!isTrinoMaterializedView(getTableType(table), firstNonNull(table.getParameters(), ImmutableMap.of()))) {
return Optional.empty();
}

Expand Down Expand Up @@ -956,7 +958,7 @@ public void renameMaterializedView(ConnectorSession session, SchemaTableName sou
com.amazonaws.services.glue.model.Table glueTable = getTable(session, source)
.orElseThrow(() -> new TableNotFoundException(source));
materializedViewCache.remove(source);
if (!isTrinoMaterializedView(glueTable.getTableType(), firstNonNull(glueTable.getParameters(), ImmutableMap.of()))) {
if (!isTrinoMaterializedView(getTableType(glueTable), firstNonNull(glueTable.getParameters(), ImmutableMap.of()))) {
throw new TrinoException(UNSUPPORTED_TABLE_TYPE, "Not a Materialized View: " + source);
}
TableInput tableInput = getMaterializedViewTableInput(target.getTableName(), glueTable.getViewOriginalText(), glueTable.getOwner(), glueTable.getParameters());
Expand Down Expand Up @@ -1003,7 +1005,7 @@ public Optional<CatalogSchemaTableName> redirectTable(ConnectorSession session,

Optional<com.amazonaws.services.glue.model.Table> table = getTable(session, new SchemaTableName(tableNameBase.getSchemaName(), tableNameBase.getTableName()));

if (table.isEmpty() || VIRTUAL_VIEW.name().equals(table.get().getTableType())) {
if (table.isEmpty() || VIRTUAL_VIEW.name().equals(getTableTypeNullable(table.get()))) {
return Optional.empty();
}
if (!isIcebergTable(firstNonNull(table.get().getParameters(), ImmutableMap.of()))) {
Expand Down
3 changes: 2 additions & 1 deletion ...rc/main/java/io/trino/tests/product/deltalake/TestDatabricksWithGlueMetastoreCleanUp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.stream.Collectors;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableType;
import static io.trino.tests.product.TestGroups.DELTA_LAKE_DATABRICKS;
import static io.trino.tests.product.TestGroups.PROFILE_SPECIFIC_TESTS;
import static io.trino.tests.product.deltalake.util.DeltaLakeTestUtils.DATABRICKS_COMMUNICATION_FAILURE_ISSUE;
Expand Down Expand Up @@ -78,7 +79,7 @@ private void cleanSchema(String schema, long startTime, AWSGlueAsync glueClient)
Table table = glueClient.getTable(new GetTableRequest().withDatabaseName(schema).withName(tableName)).getTable();
Instant createTime = table.getCreateTime().toInstant();
if (createTime.isBefore(SCHEMA_CLEANUP_THRESHOLD)) {
if (table.getTableType() != null && table.getTableType().contains("VIEW")) {
if (getTableType(table).contains("VIEW")) {
onTrino().executeQuery(format("DROP VIEW IF EXISTS %s.%s", schema, tableName));
log.info("Dropped view %s.%s", schema, tableName);
}
Expand Down