Skip to content
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

Remove duplicate logic for create/update Iceberg table in metastore #18500

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Remove duplicate logic for create/update Iceberg table in metastore #18500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77eb956
Propagate Iceberg comment to metastore on modifications
findepi Aug 2, 2023
bae338b
Remove duplicate logic for create/update Iceberg table in metastore
findepi Aug 2, 2023
dbf6d8e
Update variable name
findepi Aug 2, 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
17 changes: 17 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.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.OptionalLong;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.stream.Stream;

import static com.google.common.base.MoreObjects.toStringHelper;
Expand Down Expand Up @@ -327,6 +328,17 @@ public Builder setParameter(String key, String value)
return this;
}

public Builder setParameter(String key, Optional<String> value)
{
if (value.isEmpty()) {
this.parameters.remove(key);
}
else {
this.parameters.put(key, value.get());
}
return this;
}

public Builder setViewOriginalText(Optional<String> viewOriginalText)
{
this.viewOriginalText = viewOriginalText;
Expand All @@ -351,6 +363,11 @@ public Builder withStorage(Consumer<Storage.Builder> consumer)
return this;
}

public Builder apply(Function<Builder, Builder> function)
{
return function.apply(this);
}

public Table build()
{
return new Table(
Expand Down
6 changes: 1 addition & 5 deletions ...berg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import static io.trino.plugin.hive.HiveErrorCode.HIVE_CONCURRENT_MODIFICATION_DETECTED;
import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES;
import static org.apache.iceberg.BaseMetastoreTableOperations.METADATA_LOCATION_PROP;
import static org.apache.iceberg.BaseMetastoreTableOperations.PREVIOUS_METADATA_LOCATION_PROP;

@NotThreadSafe
public class FileMetastoreTableOperations
Expand Down Expand Up @@ -65,10 +64,7 @@ protected void commitToExistingTable(TableMetadata base, TableMetadata metadata)
String newMetadataLocation = writeNewMetadata(metadata, version.orElseThrow() + 1);

Table table = Table.builder(currentTable)
.setDataColumns(toHiveColumns(metadata.schema().columns()))
.withStorage(storage -> storage.setLocation(metadata.location()))
.setParameter(METADATA_LOCATION_PROP, newMetadataLocation)
.setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation)
.apply(builder -> updateMetastoreTable(builder, metadata, newMetadataLocation, Optional.of(currentMetadataLocation)))
.build();

// todo privileges should not be replaced for an alter
Expand Down
8 changes: 4 additions & 4 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private static Optional<List<Column>> glueColumns(TypeManager typeManager, Table
glueTypeString.length() > GLUE_COLUMN_TYPE_LENGTH_LIMIT) {
return Optional.empty();
}
String trinoTypeName = TypeConverter.toTrinoType(icebergColumn.type(), typeManager).getTypeId().getId();
String trinoTypeId = TypeConverter.toTrinoType(icebergColumn.type(), typeManager).getTypeId().getId();
Column column = new Column()
.withName(icebergColumn.name())
.withType(glueTypeString)
Expand All @@ -133,12 +133,12 @@ private static Optional<List<Column>> glueColumns(TypeManager typeManager, Table
if (icebergColumn.isRequired()) {
parameters.put(COLUMN_TRINO_NOT_NULL_PROPERTY, "true");
}
if (firstColumn || !glueTypeString.equals(trinoTypeName)) {
if (trinoTypeName.length() > GLUE_COLUMN_PARAMETER_LENGTH_LIMIT) {
if (firstColumn || !glueTypeString.equals(trinoTypeId)) {
if (trinoTypeId.length() > GLUE_COLUMN_PARAMETER_LENGTH_LIMIT) {
return Optional.empty();
}
// Store type parameter for some (first) column so that we can later detect whether column parameters weren't erased by something.
parameters.put(COLUMN_TRINO_TYPE_ID_PROPERTY, trinoTypeName);
parameters.put(COLUMN_TRINO_TYPE_ID_PROPERTY, trinoTypeId);
}
column.setParameters(parameters.buildOrThrow());
glueColumns.add(column);
Expand Down
4 changes: 2 additions & 2 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 @@ -431,9 +431,9 @@ private Optional<List<ColumnMetadata>> getColumnMetadata(SchemaTableName tableNa
ImmutableList.Builder<ColumnMetadata> columns = ImmutableList.builderWithExpectedSize(glueColumns.size());
for (Column glueColumn : glueColumns) {
Map<String, String> columnParameters = getColumnParameters(glueColumn);
String trinoTypeName = columnParameters.getOrDefault(COLUMN_TRINO_TYPE_ID_PROPERTY, glueColumn.getType());
String trinoTypeId = columnParameters.getOrDefault(COLUMN_TRINO_TYPE_ID_PROPERTY, glueColumn.getType());
boolean notNull = parseBoolean(columnParameters.getOrDefault(COLUMN_TRINO_NOT_NULL_PROPERTY, "false"));
Type type = typeManager.getType(TypeId.of(trinoTypeName));
Type type = typeManager.getType(TypeId.of(trinoTypeId));
columns.add(ColumnMetadata.builder()
.setName(glueColumn.getName())
.setType(type)
Expand Down
23 changes: 14 additions & 9 deletions ...g/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import static java.util.Objects.requireNonNull;
import static org.apache.iceberg.BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE;
import static org.apache.iceberg.BaseMetastoreTableOperations.METADATA_LOCATION_PROP;
import static org.apache.iceberg.BaseMetastoreTableOperations.PREVIOUS_METADATA_LOCATION_PROP;
import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP;

@NotThreadSafe
Expand Down Expand Up @@ -94,24 +95,18 @@ protected final void commitNewTable(TableMetadata metadata)
verify(version.isEmpty(), "commitNewTable called on a table which already exists");
String newMetadataLocation = writeNewMetadata(metadata, 0);

Table.Builder builder = Table.builder()
Table table = Table.builder()
.setDatabaseName(database)
.setTableName(tableName)
.setOwner(owner)
// Table needs to be EXTERNAL, otherwise table rename in HMS would rename table directory and break table contents.
.setTableType(EXTERNAL_TABLE.name())
.setDataColumns(toHiveColumns(metadata.schema().columns()))
.withStorage(storage -> storage.setLocation(metadata.location()))
.withStorage(storage -> storage.setStorageFormat(ICEBERG_METASTORE_STORAGE_FORMAT))
// This is a must-have property for the EXTERNAL_TABLE table type
.setParameter("EXTERNAL", "TRUE")
.setParameter(TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE.toUpperCase(ENGLISH))
.setParameter(METADATA_LOCATION_PROP, newMetadataLocation);
String tableComment = metadata.properties().get(TABLE_COMMENT);
if (tableComment != null) {
builder.setParameter(TABLE_COMMENT, tableComment);
}
Table table = builder.build();
.apply(builder -> updateMetastoreTable(builder, metadata, newMetadataLocation, Optional.empty()))
.build();

PrincipalPrivileges privileges = owner.map(MetastoreUtil::buildInitialPrivilegeSet).orElse(NO_PRIVILEGES);
try {
Expand All @@ -125,6 +120,16 @@ protected final void commitNewTable(TableMetadata metadata)
}
}

protected Table.Builder updateMetastoreTable(Table.Builder builder, TableMetadata metadata, String metadataLocation, Optional<String> previousMetadataLocation)
{
return builder
.setDataColumns(toHiveColumns(metadata.schema().columns()))
.withStorage(storage -> storage.setLocation(metadata.location()))
.setParameter(METADATA_LOCATION_PROP, metadataLocation)
.setParameter(PREVIOUS_METADATA_LOCATION_PROP, previousMetadataLocation)
.setParameter(TABLE_COMMENT, Optional.ofNullable(metadata.properties().get(TABLE_COMMENT)));
}

protected Table getTable()
{
return metastore.getTable(database, tableName)
Expand Down
6 changes: 1 addition & 5 deletions ...eberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import static io.trino.plugin.iceberg.IcebergUtil.fixBrokenMetadataLocation;
import static java.util.Objects.requireNonNull;
import static org.apache.iceberg.BaseMetastoreTableOperations.METADATA_LOCATION_PROP;
import static org.apache.iceberg.BaseMetastoreTableOperations.PREVIOUS_METADATA_LOCATION_PROP;

@NotThreadSafe
public class HiveMetastoreTableOperations
Expand Down Expand Up @@ -81,10 +80,7 @@ protected void commitToExistingTable(TableMetadata base, TableMetadata metadata)
}

Table table = Table.builder(currentTable)
.setDataColumns(toHiveColumns(metadata.schema().columns()))
.withStorage(storage -> storage.setLocation(metadata.location()))
.setParameter(METADATA_LOCATION_PROP, newMetadataLocation)
.setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation)
.apply(builder -> updateMetastoreTable(builder, metadata, newMetadataLocation, Optional.of(currentMetadataLocation)))
.build();

// todo privileges should not be replaced for an alter
Expand Down