From 77eb95639df0822bc9f07402ab216cd57928f429 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 2 Aug 2023 14:54:54 +0200 Subject: [PATCH 1/3] Propagate Iceberg comment to metastore on modifications `AbstractMetastoreTableOperations.commitNewTable` sets `comment` table parameter. Update it on table changes too. --- .../java/io/trino/plugin/hive/metastore/Table.java | 11 +++++++++++ .../catalog/file/FileMetastoreTableOperations.java | 2 ++ .../catalog/hms/HiveMetastoreTableOperations.java | 2 ++ 3 files changed, 15 insertions(+) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java index 6ffb133d479c..6d7d9a175960 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java @@ -327,6 +327,17 @@ public Builder setParameter(String key, String value) return this; } + public Builder setParameter(String key, Optional value) + { + if (value.isEmpty()) { + this.parameters.remove(key); + } + else { + this.parameters.put(key, value.get()); + } + return this; + } + public Builder setViewOriginalText(Optional viewOriginalText) { this.viewOriginalText = viewOriginalText; diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java index 7b66a3d3e798..d16f72ed3275 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java @@ -30,6 +30,7 @@ import static com.google.common.base.Preconditions.checkState; import static io.trino.plugin.hive.HiveErrorCode.HIVE_CONCURRENT_MODIFICATION_DETECTED; +import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT; 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; @@ -69,6 +70,7 @@ protected void commitToExistingTable(TableMetadata base, TableMetadata metadata) .withStorage(storage -> storage.setLocation(metadata.location())) .setParameter(METADATA_LOCATION_PROP, newMetadataLocation) .setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation) + .setParameter(TABLE_COMMENT, Optional.ofNullable(metadata.properties().get(TABLE_COMMENT))) .build(); // todo privileges should not be replaced for an alter diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java index 7a78a9eb0c70..18f23db2dbbd 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java @@ -31,6 +31,7 @@ import java.util.Optional; import static com.google.common.base.Preconditions.checkState; +import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT; import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES; import static io.trino.plugin.hive.metastore.thrift.ThriftMetastoreUtil.fromMetastoreApiTable; import static io.trino.plugin.iceberg.IcebergUtil.fixBrokenMetadataLocation; @@ -85,6 +86,7 @@ protected void commitToExistingTable(TableMetadata base, TableMetadata metadata) .withStorage(storage -> storage.setLocation(metadata.location())) .setParameter(METADATA_LOCATION_PROP, newMetadataLocation) .setParameter(PREVIOUS_METADATA_LOCATION_PROP, currentMetadataLocation) + .setParameter(TABLE_COMMENT, Optional.ofNullable(metadata.properties().get(TABLE_COMMENT))) .build(); // todo privileges should not be replaced for an alter From bae338b245ee67ff20995a34f0da3d38af2381fb Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 2 Aug 2023 15:11:35 +0200 Subject: [PATCH 2/3] Remove duplicate logic for create/update Iceberg table in metastore Deduplicate logic for `AbstractMetastoreTableOperations.commitNewTable` and `commitToExistingTable` for updating metastore `Table` from Iceberg `TableMetadata`. --- .../io/trino/plugin/hive/metastore/Table.java | 6 +++++ .../file/FileMetastoreTableOperations.java | 8 +------ .../hms/AbstractMetastoreTableOperations.java | 23 +++++++++++-------- .../hms/HiveMetastoreTableOperations.java | 8 +------ 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java index 6d7d9a175960..ba48867d8f7c 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Table.java @@ -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; @@ -362,6 +363,11 @@ public Builder withStorage(Consumer consumer) return this; } + public Builder apply(Function function) + { + return function.apply(this); + } + public Table build() { return new Table( diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java index d16f72ed3275..e25fac198f77 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java @@ -30,10 +30,8 @@ import static com.google.common.base.Preconditions.checkState; import static io.trino.plugin.hive.HiveErrorCode.HIVE_CONCURRENT_MODIFICATION_DETECTED; -import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT; 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 @@ -66,11 +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) - .setParameter(TABLE_COMMENT, Optional.ofNullable(metadata.properties().get(TABLE_COMMENT))) + .apply(builder -> updateMetastoreTable(builder, metadata, newMetadataLocation, Optional.of(currentMetadataLocation))) .build(); // todo privileges should not be replaced for an alter diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java index 3d7291cd64e4..0774d7103c2e 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java @@ -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 @@ -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 { @@ -125,6 +120,16 @@ protected final void commitNewTable(TableMetadata metadata) } } + protected Table.Builder updateMetastoreTable(Table.Builder builder, TableMetadata metadata, String metadataLocation, Optional 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) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java index 18f23db2dbbd..5a5d4dead2b1 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java @@ -31,13 +31,11 @@ import java.util.Optional; import static com.google.common.base.Preconditions.checkState; -import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT; import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES; import static io.trino.plugin.hive.metastore.thrift.ThriftMetastoreUtil.fromMetastoreApiTable; 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 @@ -82,11 +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) - .setParameter(TABLE_COMMENT, Optional.ofNullable(metadata.properties().get(TABLE_COMMENT))) + .apply(builder -> updateMetastoreTable(builder, metadata, newMetadataLocation, Optional.of(currentMetadataLocation))) .build(); // todo privileges should not be replaced for an alter From dbf6d8e13bbdce3c06c347aaea6112bb5d71da8b Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 2 Aug 2023 15:25:38 +0200 Subject: [PATCH 3/3] Update variable name The previous name was a left-over from previous implementation attempt. --- .../plugin/iceberg/catalog/glue/GlueIcebergUtil.java | 8 ++++---- .../plugin/iceberg/catalog/glue/TrinoGlueCatalog.java | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java index d5336b14cda9..b8988d3112d2 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java @@ -123,7 +123,7 @@ private static Optional> 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) @@ -133,12 +133,12 @@ private static Optional> 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); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java index de7a94d8dfb6..2b0ce7cb3454 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java @@ -431,9 +431,9 @@ private Optional> getColumnMetadata(SchemaTableName tableNa ImmutableList.Builder columns = ImmutableList.builderWithExpectedSize(glueColumns.size()); for (Column glueColumn : glueColumns) { Map 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)