From 77a9c45416fc55dc42c542f2e4b6cd844dd4a038 Mon Sep 17 00:00:00 2001 From: Marius Grama Date: Wed, 16 Feb 2022 21:11:21 +0100 Subject: [PATCH 1/3] Add redirection awareness for ADD COLUMN task --- .../io/trino/execution/AddColumnTask.java | 20 +++++++++---------- .../hive/TestHiveRedirectionToIceberg.java | 10 +++++++--- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java index e8789c36e160..5dd380551268 100644 --- a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java @@ -19,6 +19,7 @@ import io.trino.execution.warnings.WarningCollector; import io.trino.metadata.ColumnPropertyManager; import io.trino.metadata.QualifiedObjectName; +import io.trino.metadata.RedirectionAwareTableHandle; import io.trino.metadata.TableHandle; import io.trino.security.AccessControl; import io.trino.spi.connector.ColumnHandle; @@ -34,7 +35,6 @@ import java.util.List; import java.util.Map; -import java.util.Optional; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static io.trino.metadata.MetadataUtil.createQualifiedObjectName; @@ -81,20 +81,20 @@ public ListenableFuture execute( WarningCollector warningCollector) { Session session = stateMachine.getSession(); - QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName()); - Optional tableHandle = plannerContext.getMetadata().getTableHandle(session, tableName); - if (tableHandle.isEmpty()) { + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName()); + RedirectionAwareTableHandle redirectionAwareTableHandle = plannerContext.getMetadata().getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { if (!statement.isTableExists()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName); + throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", originalTableName); } return immediateVoidFuture(); } + TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); + CatalogName catalogName = getRequiredCatalogHandle(plannerContext.getMetadata(), session, statement, tableHandle.getCatalogName().getCatalogName()); - CatalogName catalogName = getRequiredCatalogHandle(plannerContext.getMetadata(), session, statement, tableName.getCatalogName()); + accessControl.checkCanAddColumns(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); - accessControl.checkCanAddColumns(session.toSecurityContext(), tableName); - - Map columnHandles = plannerContext.getMetadata().getColumnHandles(session, tableHandle.get()); + Map columnHandles = plannerContext.getMetadata().getColumnHandles(session, tableHandle); ColumnDefinition element = statement.getColumn(); Type type; @@ -133,7 +133,7 @@ public ListenableFuture execute( .setProperties(columnProperties) .build(); - plannerContext.getMetadata().addColumn(session, tableHandle.get(), column); + plannerContext.getMetadata().addColumn(session, tableHandle, column); return immediateVoidFuture(); } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java index 4824f3c4caff..17ccad1593c9 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java @@ -321,10 +321,14 @@ public void testAlterTableAddColumn() createIcebergTable(icebergTableName, false); - //TODO restore test assertions after adding redirection awareness to the AddColumnTask - assertQueryFailure(() -> onTrino().executeQuery("ALTER TABLE " + hiveTableName + " ADD COLUMN some_new_column double")) - .hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); + onTrino().executeQuery("ALTER TABLE " + hiveTableName + " ADD COLUMN some_new_column double"); + + Assertions.assertThat(onTrino().executeQuery("DESCRIBE " + icebergTableName).column(1)) + .containsOnly("nationkey", "name", "regionkey", "comment", "some_new_column"); + assertResultsEqual( + onTrino().executeQuery("TABLE " + icebergTableName), + onTrino().executeQuery("SELECT * , NULL FROM tpch.tiny.nation")); onTrino().executeQuery("DROP TABLE " + icebergTableName); } From 56e9fcf0ed4f2dd4e26280374456078d83d6cb43 Mon Sep 17 00:00:00 2001 From: Marius Grama Date: Wed, 16 Feb 2022 21:13:19 +0100 Subject: [PATCH 2/3] Add redirection awareness for COMMENT task --- .../java/io/trino/execution/CommentTask.java | 30 +++++++------ .../hive/TestHiveRedirectionToIceberg.java | 44 +++++++++++++++++-- 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/execution/CommentTask.java b/core/trino-main/src/main/java/io/trino/execution/CommentTask.java index 1e482f350b91..1b8dfde93ae3 100644 --- a/core/trino-main/src/main/java/io/trino/execution/CommentTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/CommentTask.java @@ -18,6 +18,7 @@ import io.trino.execution.warnings.WarningCollector; import io.trino.metadata.Metadata; import io.trino.metadata.QualifiedObjectName; +import io.trino.metadata.RedirectionAwareTableHandle; import io.trino.metadata.TableHandle; import io.trino.security.AccessControl; import io.trino.spi.connector.ColumnHandle; @@ -69,15 +70,15 @@ public ListenableFuture execute( Session session = stateMachine.getSession(); if (statement.getType() == Comment.Type.TABLE) { - QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName()); - Optional tableHandle = metadata.getTableHandle(session, tableName); - if (tableHandle.isEmpty()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", tableName); + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName()); + RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { + throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName); } - accessControl.checkCanSetTableComment(session.toSecurityContext(), tableName); - - metadata.setTableComment(session, tableHandle.get(), statement.getComment()); + accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); + TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); + metadata.setTableComment(session, tableHandle, statement.getComment()); } else if (statement.getType() == Comment.Type.COLUMN) { Optional prefix = statement.getName().getPrefix(); @@ -85,21 +86,22 @@ else if (statement.getType() == Comment.Type.COLUMN) { throw semanticException(MISSING_TABLE, statement, "Table must be specified"); } - QualifiedObjectName tableName = createQualifiedObjectName(session, statement, prefix.get()); - Optional tableHandle = metadata.getTableHandle(session, tableName); - if (tableHandle.isEmpty()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + tableName); + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get()); + RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { + throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName); } + TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get(); String columnName = statement.getName().getSuffix(); - Map columnHandles = metadata.getColumnHandles(session, tableHandle.get()); + Map columnHandles = metadata.getColumnHandles(session, tableHandle); if (!columnHandles.containsKey(columnName)) { throw semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName); } - accessControl.checkCanSetColumnComment(session.toSecurityContext(), tableName); + accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName)); - metadata.setColumnComment(session, tableHandle.get(), columnHandles.get(columnName), statement.getComment()); + metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment()); } else { throw semanticException(NOT_SUPPORTED, statement, "Unsupported comment type: %s", statement.getType()); diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java index 17ccad1593c9..530d0878cd7c 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java @@ -344,9 +344,32 @@ public void testCommentTable() assertTableComment("hive", "default", tableName).isNull(); assertTableComment("iceberg", "default", tableName).isNull(); - //TODO restore test assertions after adding redirection awareness to the CommentTask - assertQueryFailure(() -> onTrino().executeQuery("COMMENT ON TABLE " + hiveTableName + " IS 'This is my table, there are many like it but this one is mine'")) - .hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); + String tableComment = "This is my table, there are many like it but this one is mine"; + onTrino().executeQuery(format("COMMENT ON TABLE " + hiveTableName + " IS '%s'", tableComment)); + + assertTableComment("hive", "default", tableName).isEqualTo(tableComment); + assertTableComment("iceberg", "default", tableName).isEqualTo(tableComment); + + onTrino().executeQuery("DROP TABLE " + icebergTableName); + } + + @Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS}) + public void testCommentColumn() + { + String tableName = "iceberg_comment_column_" + randomTableSuffix(); + String hiveTableName = "hive.default." + tableName; + String icebergTableName = "iceberg.default." + tableName; + String columnName = "nationkey"; + createIcebergTable(icebergTableName, false); + + assertColumnComment("hive", "default", tableName, columnName).isNull(); + assertColumnComment("iceberg", "default", tableName, columnName).isNull(); + + String columnComment = "Internal identifier for the nation"; + onTrino().executeQuery(format("COMMENT ON COLUMN %s.%s IS '%s'", hiveTableName, columnName, columnComment)); + + assertColumnComment("hive", "default", tableName, columnName).isEqualTo(columnComment); + assertColumnComment("iceberg", "default", tableName, columnName).isEqualTo(columnComment); onTrino().executeQuery("DROP TABLE " + icebergTableName); } @@ -468,6 +491,21 @@ private static QueryResult readTableComment(String catalog, String schema, Strin param(VARCHAR, tableName)); } + private static AbstractStringAssert assertColumnComment(String catalog, String schema, String tableName, String columnName) + { + QueryResult queryResult = readColumnComment(catalog, schema, tableName, columnName); + return Assertions.assertThat((String) getOnlyElement(getOnlyElement(queryResult.rows()))); + } + + private static QueryResult readColumnComment(String catalog, String schema, String tableName, String columnName) + { + return onTrino().executeQuery( + format("SELECT comment FROM %s.information_schema.columns WHERE table_schema = ? AND table_name = ? AND column_name = ?", catalog), + param(VARCHAR, schema), + param(VARCHAR, tableName), + param(VARCHAR, columnName)); + } + private static void assertResultsEqual(QueryResult first, QueryResult second) { assertThat(first).containsOnly(second.rows().stream() From 86ad52ab66f6e7e80b8111c5547a381b6d6d348f Mon Sep 17 00:00:00 2001 From: Marius Grama Date: Wed, 16 Feb 2022 21:16:14 +0100 Subject: [PATCH 3/3] Add redirection awareness for DROP TABLE task --- .../io/trino/execution/DropTableTask.java | 24 +++++++++---------- .../hive/TestHiveRedirectionToIceberg.java | 6 ++--- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/execution/DropTableTask.java b/core/trino-main/src/main/java/io/trino/execution/DropTableTask.java index 2b6b8bad049b..cd485ec1b213 100644 --- a/core/trino-main/src/main/java/io/trino/execution/DropTableTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/DropTableTask.java @@ -18,7 +18,7 @@ import io.trino.execution.warnings.WarningCollector; import io.trino.metadata.Metadata; import io.trino.metadata.QualifiedObjectName; -import io.trino.metadata.TableHandle; +import io.trino.metadata.RedirectionAwareTableHandle; import io.trino.security.AccessControl; import io.trino.sql.tree.DropTable; import io.trino.sql.tree.Expression; @@ -26,7 +26,6 @@ import javax.inject.Inject; import java.util.List; -import java.util.Optional; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static io.trino.metadata.MetadataUtil.createQualifiedObjectName; @@ -61,39 +60,38 @@ public ListenableFuture execute( WarningCollector warningCollector) { Session session = stateMachine.getSession(); - QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName()); - - if (metadata.isMaterializedView(session, tableName)) { + QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getTableName()); + if (metadata.isMaterializedView(session, originalTableName)) { if (!statement.isExists()) { throw semanticException( TABLE_NOT_FOUND, statement, - "Table '%s' does not exist, but a materialized view with that name exists. Did you mean DROP MATERIALIZED VIEW %s?", tableName, tableName); + "Table '%s' does not exist, but a materialized view with that name exists. Did you mean DROP MATERIALIZED VIEW %s?", originalTableName, originalTableName); } return immediateVoidFuture(); } - if (metadata.isView(session, tableName)) { + if (metadata.isView(session, originalTableName)) { if (!statement.isExists()) { throw semanticException( TABLE_NOT_FOUND, statement, - "Table '%s' does not exist, but a view with that name exists. Did you mean DROP VIEW %s?", tableName, tableName); + "Table '%s' does not exist, but a view with that name exists. Did you mean DROP VIEW %s?", originalTableName, originalTableName); } return immediateVoidFuture(); } - Optional tableHandle = metadata.getTableHandle(session, tableName); - if (tableHandle.isEmpty()) { + RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName); + if (redirectionAwareTableHandle.getTableHandle().isEmpty()) { if (!statement.isExists()) { - throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", tableName); + throw semanticException(TABLE_NOT_FOUND, statement, "Table '%s' does not exist", originalTableName); } return immediateVoidFuture(); } - + QualifiedObjectName tableName = redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName); accessControl.checkCanDropTable(session.toSecurityContext(), tableName); - metadata.dropTable(session, tableHandle.get()); + metadata.dropTable(session, redirectionAwareTableHandle.getTableHandle().get()); return immediateVoidFuture(); } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java index 530d0878cd7c..0ce30a4aa016 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java @@ -230,9 +230,9 @@ public void testDropTable() String icebergTableName = "iceberg.default." + tableName; createIcebergTable(icebergTableName, false); - //TODO restore test assertions after adding redirection awareness to the DropTableTask - assertQueryFailure(() -> onTrino().executeQuery("DROP TABLE " + hiveTableName)) - .hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); + onTrino().executeQuery("DROP TABLE " + hiveTableName); + assertQueryFailure(() -> onTrino().executeQuery("TABLE " + icebergTableName)) + .hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): line 1:1: Table '" + icebergTableName + "' does not exist"); } @Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS})