Skip to content

Handle missing MV StorageDescriptor in Glue #17383

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 5 commits into from
May 10, 2023
Merged

Handle missing MV StorageDescriptor in Glue #17383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63171b9
Handle missing MV StorageDescriptor in Glue
grantatspothero Apr 28, 2023
1a8a094
Remove incorrect Glue convertParameters call
findepi May 9, 2023
9619f68
Guard against nullable parameters in Glue data model
findepi May 9, 2023
caec2f6
Prevent accessing MVs as views in Hive
findepi May 9, 2023
2610567
empty
findepi May 9, 2023
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
18 changes: 18 additions & 0 deletions .mvn/modernizer/violations.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,24 @@
<comment>Table type is nullable in Glue model, which is too easy to forget about. Prefer GlueToTrinoConverter.getTableType</comment>
</violation>

<violation>
<name>com/amazonaws/services/glue/model/Table.getParameters:()Ljava/util/Map;</name>
<version>1.1</version>
<comment>Table parameters map is nullable in Glue model, which is too easy to forget about. Prefer GlueToTrinoConverter.getTableParameters</comment>
</violation>

<violation>
<name>com/amazonaws/services/glue/model/Partition.getParameters:()Ljava/util/Map;</name>
<version>1.1</version>
<comment>Partition parameters map is nullable in Glue model, which is too easy to forget about. Prefer GlueToTrinoConverter.getPartitionParameters</comment>
</violation>

<violation>
<name>com/amazonaws/services/glue/model/SerDeInfo.getParameters:()Ljava/util/Map;</name>
<version>1.1</version>
<comment>SerDeInfo parameters map is nullable in Glue model, which is too easy to forget about. Prefer GlueToTrinoConverter.getSerDeInfoParameters</comment>
</violation>

<violation>
<name>org/apache/hadoop/mapred/JobConf."&lt;init&gt;":()V</name>
<version>1.1</version>
Expand Down
9 changes: 8 additions & 1 deletion plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,22 @@ public static boolean isHiveOrPrestoView(String tableType)
return tableType.equals(VIRTUAL_VIEW.name());
}

public static boolean isTrinoMaterializedView(Table table)
{
return isTrinoMaterializedView(table.getTableType(), table.getParameters());
}

public static boolean isTrinoMaterializedView(String tableType, Map<String, String> tableParameters)
{
// TODO isHiveOrPrestoView should not return true for materialized views
return isHiveOrPrestoView(tableType) && isPrestoView(tableParameters) && tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(ICEBERG_MATERIALIZED_VIEW_COMMENT);
}

public static boolean canDecodeView(Table table)
{
// we can decode Hive or Presto view
return table.getTableType().equals(VIRTUAL_VIEW.name());
return table.getTableType().equals(VIRTUAL_VIEW.name()) &&
!isTrinoMaterializedView(table.getTableType(), table.getParameters());
Copy link
Copy Markdown
Member Author

@findepi findepi May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was proposed in #15221 but back then we couldn't test it. Now we can thanks to the test added by Grant.
canDecodeView should be removed (#17276 (comment)), leaving this to a follow-up (#17276).

}

public static String encodeViewData(ConnectorViewDefinition definition)
Expand Down
4 changes: 2 additions & 2 deletions ...ain/java/io/trino/plugin/hive/metastore/glue/DefaultGlueMetastoreTableFilterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import java.util.Map;
import java.util.function.Predicate;

import static com.google.common.base.MoreObjects.firstNonNull;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableParameters;
import static io.trino.plugin.hive.util.HiveUtil.DELTA_LAKE_PROVIDER;
import static io.trino.plugin.hive.util.HiveUtil.SPARK_TABLE_PROVIDER_KEY;
import static java.util.function.Predicate.not;
Expand All @@ -49,7 +49,7 @@ public Predicate<Table> get()

public static boolean isDeltaLakeTable(Table table)
{
Map<String, String> parameters = firstNonNull(table.getParameters(), Map.of());
Map<String, String> parameters = getTableParameters(table);
return parameters.getOrDefault(SPARK_TABLE_PROVIDER_KEY, "").equalsIgnoreCase(DELTA_LAKE_PROVIDER);
}
}
6 changes: 3 additions & 3 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 @@ -132,7 +132,6 @@
import java.util.function.Function;
import java.util.function.Predicate;

import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.Comparators.lexicographical;
Expand All @@ -149,6 +148,7 @@
import static io.trino.plugin.hive.metastore.glue.AwsSdkUtil.getPaginatedResults;
import static io.trino.plugin.hive.metastore.glue.GlueClientUtil.createAsyncGlueClient;
import static io.trino.plugin.hive.metastore.glue.converter.GlueInputConverter.convertPartition;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableParameters;
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;
Expand Down Expand Up @@ -442,7 +442,7 @@ public List<String> getAllTables(String databaseName)
@Override
public synchronized List<String> getTablesWithParameter(String databaseName, String parameterKey, String parameterValue)
{
return getAllViews(databaseName, table -> parameterValue.equals(firstNonNull(table.getParameters(), ImmutableMap.of()).get(parameterKey)));
return getAllViews(databaseName, table -> parameterValue.equals(getTableParameters(table).get(parameterKey)));
}

@Override
Expand Down Expand Up @@ -680,7 +680,7 @@ private TableInput convertGlueTableToTableInput(com.amazonaws.services.glue.mode
.withViewExpandedText(glueTable.getViewExpandedText())
.withTableType(getTableTypeNullable(glueTable))
.withTargetTable(glueTable.getTargetTable())
.withParameters(glueTable.getParameters());
.withParameters(getTableParameters(glueTable));
}

@Override
Expand Down
45 changes: 30 additions & 15 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 @@ -51,6 +51,7 @@
import static io.trino.plugin.hive.HiveErrorCode.HIVE_UNSUPPORTED_FORMAT;
import static io.trino.plugin.hive.HiveType.HIVE_INT;
import static io.trino.plugin.hive.TableType.EXTERNAL_TABLE;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView;
import static io.trino.plugin.hive.metastore.util.Memoizers.memoizeLast;
import static io.trino.plugin.hive.util.HiveUtil.isDeltaLakeTable;
import static io.trino.plugin.hive.util.HiveUtil.isIcebergTable;
Expand All @@ -76,6 +77,24 @@ public static String getTableTypeNullable(com.amazonaws.services.glue.model.Tabl
return glueTable.getTableType();
}

@SuppressModernizer // Usage of `Table.getParameters` is not allowed. Only this method can call that.
public static Map<String, String> getTableParameters(com.amazonaws.services.glue.model.Table glueTable)
{
return firstNonNull(glueTable.getParameters(), ImmutableMap.of());
}

@SuppressModernizer // Usage of `Partition.getParameters` is not allowed. Only this method can call that.
public static Map<String, String> getPartitionParameters(com.amazonaws.services.glue.model.Partition gluePartition)
{
return firstNonNull(gluePartition.getParameters(), ImmutableMap.of());
}

@SuppressModernizer // Usage of `SerDeInfo.getParameters` is not allowed. Only this method can call that.
public static Map<String, String> getSerDeInfoParameters(com.amazonaws.services.glue.model.SerDeInfo glueSerDeInfo)
{
return firstNonNull(glueSerDeInfo.getParameters(), ImmutableMap.of());
}

public static Database convertDatabase(com.amazonaws.services.glue.model.Database glueDb)
{
return Database.builder()
Expand All @@ -95,21 +114,25 @@ public static Table convertTable(com.amazonaws.services.glue.model.Table glueTab
{
SchemaTableName table = new SchemaTableName(dbName, glueTable.getName());

Map<String, String> tableParameters = convertParameters(glueTable.getParameters());
String tableType = getTableType(glueTable);
Map<String, String> tableParameters = ImmutableMap.copyOf(getTableParameters(glueTable));
Table.Builder tableBuilder = Table.builder()
.setDatabaseName(table.getSchemaName())
.setTableName(table.getTableName())
.setOwner(Optional.ofNullable(glueTable.getOwner()))
.setTableType(getTableType(glueTable))
.setTableType(tableType)
.setParameters(tableParameters)
.setViewOriginalText(Optional.ofNullable(glueTable.getViewOriginalText()))
.setViewExpandedText(Optional.ofNullable(glueTable.getViewExpandedText()));

StorageDescriptor sd = glueTable.getStorageDescriptor();

if (isIcebergTable(tableParameters) || (sd == null && isDeltaLakeTable(tableParameters))) {
if (isIcebergTable(tableParameters) ||
(sd == null && isDeltaLakeTable(tableParameters)) ||
(sd == null && isTrinoMaterializedView(tableType, tableParameters))) {
// Iceberg tables do not need to read the StorageDescriptor field, but we still need to return dummy properties for compatibility
// Delta Lake tables only need to provide a dummy properties if a StorageDescriptor was not explicitly configured.
// Materialized views do not need to read the StorageDescriptor, but we still need to return dummy properties for compatibility
tableBuilder.setDataColumns(ImmutableList.of(new Column("dummy", HIVE_INT, Optional.empty())));
tableBuilder.getStorageBuilder().setStorageFormat(StorageFormat.fromHiveStorageFormat(HiveStorageFormat.PARQUET));
}
Expand Down Expand Up @@ -157,17 +180,9 @@ private static List<Column> convertColumns(SchemaTableName table, List<com.amazo
return mappedCopy(glueColumns, glueColumn -> convertColumn(table, glueColumn, serde));
}

private static Map<String, String> convertParameters(Map<String, String> parameters)
{
if (parameters == null || parameters.isEmpty()) {
return ImmutableMap.of();
}
return ImmutableMap.copyOf(parameters);
}

private static Function<Map<String, String>, Map<String, String>> parametersConverter()
{
return memoizeLast(GlueToTrinoConverter::convertParameters);
return memoizeLast(ImmutableMap::copyOf);
}

private static boolean isNullOrEmpty(List<?> list)
Expand All @@ -190,7 +205,7 @@ public GluePartitionConverter(Table table)
requireNonNull(table, "table is null");
this.databaseName = requireNonNull(table.getDatabaseName(), "databaseName is null");
this.tableName = requireNonNull(table.getTableName(), "tableName is null");
this.tableParameters = convertParameters(table.getParameters());
this.tableParameters = table.getParameters();
this.columnsConverter = memoizeLast(glueColumns -> convertColumns(
table.getSchemaTableName(),
glueColumns,
Expand All @@ -214,7 +229,7 @@ public Partition apply(com.amazonaws.services.glue.model.Partition gluePartition
.setTableName(tableName)
.setValues(gluePartition.getValues()) // No memoization benefit
.setColumns(columnsConverter.apply(sd.getColumns()))
.setParameters(parametersConverter.apply(gluePartition.getParameters()));
.setParameters(parametersConverter.apply(getPartitionParameters(gluePartition)));

storageConverter.setStorageBuilder(sd, partitionBuilder.getStorageBuilder(), tableParameters);

Expand All @@ -239,7 +254,7 @@ public void setStorageBuilder(StorageDescriptor sd, Storage.Builder storageBuild
.setLocation(nullToEmpty(sd.getLocation()))
.setBucketProperty(convertToBucketProperty(tableParameters, sd))
.setSkewed(sd.getSkewedInfo() != null && !isNullOrEmpty(sd.getSkewedInfo().getSkewedColumnNames()))
.setSerdeParameters(serdeParametersConverter.apply(serdeInfo.getParameters()))
.setSerdeParameters(serdeParametersConverter.apply(getSerDeInfoParameters(serdeInfo)))
.build();
}

Expand Down
18 changes: 15 additions & 3 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,9 @@
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.TestingMetastoreObjects.getGlueTestTrinoMaterializedView;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getPartitionParameters;
import static io.trino.plugin.hive.metastore.glue.converter.GlueToTrinoConverter.getTableParameters;
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;
Expand Down Expand Up @@ -96,7 +99,7 @@ public void testConvertTable()
assertEquals(trinoTable.getDatabaseName(), testDatabase.getName());
assertEquals(trinoTable.getTableType(), getTableTypeNullable(testTable));
assertEquals(trinoTable.getOwner().orElse(null), testTable.getOwner());
assertEquals(trinoTable.getParameters(), testTable.getParameters());
assertEquals(trinoTable.getParameters(), getTableParameters(testTable));
assertColumnList(trinoTable.getDataColumns(), testTable.getStorageDescriptor().getColumns());
assertColumnList(trinoTable.getPartitionColumns(), testTable.getPartitionKeys());
assertStorage(trinoTable.getStorage(), testTable.getStorageDescriptor());
Expand All @@ -117,7 +120,7 @@ public void testConvertTableWithOpenCSVSerDe()
assertEquals(trinoTable.getDatabaseName(), testDatabase.getName());
assertEquals(trinoTable.getTableType(), getTableTypeNullable(glueTable));
assertEquals(trinoTable.getOwner().orElse(null), glueTable.getOwner());
assertEquals(trinoTable.getParameters(), glueTable.getParameters());
assertEquals(trinoTable.getParameters(), getTableParameters(glueTable));
assertEquals(trinoTable.getDataColumns().size(), 1);
assertEquals(trinoTable.getDataColumns().get(0).getType(), HIVE_STRING);

Expand Down Expand Up @@ -162,7 +165,7 @@ public void testConvertPartition()
assertColumnList(trinoPartition.getColumns(), testPartition.getStorageDescriptor().getColumns());
assertEquals(trinoPartition.getValues(), testPartition.getValues());
assertStorage(trinoPartition.getStorage(), testPartition.getStorageDescriptor());
assertEquals(trinoPartition.getParameters(), testPartition.getParameters());
assertEquals(trinoPartition.getParameters(), getPartitionParameters(testPartition));
}

@Test
Expand Down Expand Up @@ -259,6 +262,15 @@ public void testDeltaTableNonNullStorageDescriptor()
.collect(toImmutableSet()));
}

@Test
public void testIcebergMaterializedViewNullStorageDescriptor()
{
Table testMaterializedView = getGlueTestTrinoMaterializedView(testDatabase.getName());
assertNull(testMaterializedView.getStorageDescriptor());
io.trino.plugin.hive.metastore.Table trinoTable = GlueToTrinoConverter.convertTable(testMaterializedView, testDatabase.getName());
assertEquals(trinoTable.getDataColumns().size(), 1);
}

@Test
public void testPartitionNullParameters()
{
Expand Down
Loading