From 7e6c1898f871d487120eb0b611450c332a9b1527 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 22 Jul 2021 17:25:29 +0200 Subject: [PATCH 1/2] Test ALTER TABLE .. RENAME for Iceberg --- .../iceberg/BaseIcebergConnectorTest.java | 2 +- .../TestIcebergConnectorSmokeTest.java | 2 +- .../iceberg/TestIcebergRenameTable.java | 55 +++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergRenameTable.java diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index 041ee03e4e82..267a70f08a59 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -188,7 +188,7 @@ public void testRowLevelDelete() public void testRenameTable() { // Iceberg table rename is not supported in FileHiveMetastore - // TODO add a test with a different metastore, or block rename in IcebergMetadata + // rename is tested with a real metastore in product tests assertThatThrownBy(super::testRenameTable) .hasStackTraceContaining("Rename not supported for Iceberg tables"); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java index b3dea8b69bba..2b441e439199 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java @@ -72,7 +72,7 @@ public void testRowLevelDelete() public void testRenameTable() { // Iceberg table rename is not supported in FileHiveMetastore - // TODO add a test with a different metastore, or block rename in IcebergMetadata + // rename is tested with a real metastore in product tests assertThatThrownBy(super::testRenameTable) .hasStackTraceContaining("Rename not supported for Iceberg tables"); } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergRenameTable.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergRenameTable.java new file mode 100644 index 000000000000..08af7c885616 --- /dev/null +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergRenameTable.java @@ -0,0 +1,55 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.tests.product.iceberg; + +import io.trino.tempto.ProductTest; +import org.testng.annotations.Test; + +import static io.trino.tempto.assertions.QueryAssert.Row.row; +import static io.trino.tempto.assertions.QueryAssert.assertThat; +import static io.trino.tests.product.TestGroups.ICEBERG; +import static io.trino.tests.product.TestGroups.STORAGE_FORMATS; +import static io.trino.tests.product.hive.util.TemporaryHiveTable.randomTableSuffix; +import static io.trino.tests.product.utils.QueryExecutors.onTrino; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class TestIcebergRenameTable + extends ProductTest +{ + @Test(groups = {ICEBERG, STORAGE_FORMATS}) + public void testRenameTable() + { + String tableName = "iceberg.default.test_rename_table_" + randomTableSuffix(); + String newName = "iceberg.default.test_rename_table_new_" + randomTableSuffix(); + onTrino().executeQuery("CREATE TABLE " + tableName + "(a bigint, b varchar)"); + try { + onTrino().executeQuery("INSERT INTO " + tableName + "(a, b) VALUES " + + "(NULL, NULL), " + + "(-42, 'abc'), " + + "(9223372036854775807, 'abcdefghijklmnopqrstuvwxyz')"); + onTrino().executeQuery("ALTER TABLE " + tableName + " RENAME TO " + newName); + assertThatThrownBy(() -> onTrino().executeQuery("SELECT * FROM " + tableName)) + .hasMessageContaining("Table '" + tableName + "' does not exist"); + assertThat(onTrino().executeQuery("SELECT * FROM " + newName)) + .containsOnly( + row(null, null), + row(-42, "abc"), + row(9223372036854775807L, "abcdefghijklmnopqrstuvwxyz")); + } + finally { + onTrino().executeQuery("DROP TABLE IF EXISTS " + tableName); + onTrino().executeQuery("DROP TABLE IF EXISTS " + newName); + } + } +} From da4ac47e9aca92acc672e4c57774d29f65062683 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 22 Jul 2021 17:51:16 +0200 Subject: [PATCH 2/2] Unblock rename in file metastore This increases non-product test test coverage for Iceberg. Added benefit is that declared connector behavior is now correct. --- .../metastore/file/FileHiveMetastore.java | 20 +++++++++++++------ .../iceberg/BaseIcebergConnectorTest.java | 11 ---------- .../TestIcebergConnectorSmokeTest.java | 11 ---------- 3 files changed, 14 insertions(+), 28 deletions(-) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java index 5989bac8ed53..561e93fa007c 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java @@ -556,10 +556,6 @@ public synchronized void renameTable(HiveIdentity identity, String databaseName, Table table = getRequiredTable(databaseName, tableName); getRequiredDatabase(newDatabaseName); - if (isIcebergTable(table)) { - throw new TrinoException(NOT_SUPPORTED, "Rename not supported for Iceberg tables"); - } - // verify new table does not exist verifyTableNotExists(newDatabaseName, newTableName); @@ -567,8 +563,20 @@ public synchronized void renameTable(HiveIdentity identity, String databaseName, Path newPath = getTableMetadataDirectory(newDatabaseName, newTableName); try { - if (!metadataFileSystem.rename(oldPath, newPath)) { - throw new TrinoException(HIVE_METASTORE_ERROR, "Could not rename table directory"); + if (isIcebergTable(table)) { + if (!metadataFileSystem.mkdirs(newPath)) { + throw new TrinoException(HIVE_METASTORE_ERROR, "Could not create new table directory"); + } + // Iceberg metadata references files in old path, so these cannot be moved. Moving table description (metadata from metastore perspective) only. + if (!metadataFileSystem.rename(new Path(oldPath, TRINO_SCHEMA_FILE_NAME), new Path(newPath, TRINO_SCHEMA_FILE_NAME))) { + throw new TrinoException(HIVE_METASTORE_ERROR, "Could not rename table schema file"); + } + // TODO drop data files when table is being dropped + } + else { + if (!metadataFileSystem.rename(oldPath, newPath)) { + throw new TrinoException(HIVE_METASTORE_ERROR, "Could not rename table directory"); + } } } catch (IOException e) { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index 267a70f08a59..f9c975f3a8ac 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -117,7 +117,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { switch (connectorBehavior) { case SUPPORTS_COMMENT_ON_COLUMN: - case SUPPORTS_RENAME_TABLE: case SUPPORTS_TOPN_PUSHDOWN: return false; @@ -183,16 +182,6 @@ public void testRowLevelDelete() .hasStackTraceContaining("This connector only supports delete where one or more partitions are deleted entirely"); } - @Test - @Override - public void testRenameTable() - { - // Iceberg table rename is not supported in FileHiveMetastore - // rename is tested with a real metastore in product tests - assertThatThrownBy(super::testRenameTable) - .hasStackTraceContaining("Rename not supported for Iceberg tables"); - } - @Test @Override public void testShowCreateSchema() diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java index 2b441e439199..3e71360e02c9 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConnectorSmokeTest.java @@ -41,7 +41,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { switch (connectorBehavior) { case SUPPORTS_COMMENT_ON_COLUMN: - case SUPPORTS_RENAME_TABLE: case SUPPORTS_TOPN_PUSHDOWN: return false; @@ -67,16 +66,6 @@ public void testRowLevelDelete() .hasStackTraceContaining("This connector only supports delete where one or more partitions are deleted entirely"); } - @Test - @Override - public void testRenameTable() - { - // Iceberg table rename is not supported in FileHiveMetastore - // rename is tested with a real metastore in product tests - assertThatThrownBy(super::testRenameTable) - .hasStackTraceContaining("Rename not supported for Iceberg tables"); - } - @Test @Override public void testShowCreateTable()