Skip to content

Add redirection awareness for ADD COLUMN, COMMENT, DROP table tasks #11072

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
phd3 merged 3 commits into from
Mar 1, 2022
Merged

Add redirection awareness for ADD COLUMN, COMMENT, DROP table tasks #11072

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
77a9c45
Add redirection awareness for ADD COLUMN task
findinpath Feb 16, 2022
56e9fcf
Add redirection awareness for COMMENT task
findinpath Feb 16, 2022
86ad52a
Add redirection awareness for DROP TABLE task
findinpath Feb 16, 2022
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
20 changes: 10 additions & 10 deletions core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -81,20 +81,20 @@ public ListenableFuture<Void> execute(
WarningCollector warningCollector)
{
Session session = stateMachine.getSession();
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
Optional<TableHandle> 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<String, ColumnHandle> columnHandles = plannerContext.getMetadata().getColumnHandles(session, tableHandle.get());
Map<String, ColumnHandle> columnHandles = plannerContext.getMetadata().getColumnHandles(session, tableHandle);

ColumnDefinition element = statement.getColumn();
Type type;
Expand Down Expand Up @@ -133,7 +133,7 @@ public ListenableFuture<Void> execute(
.setProperties(columnProperties)
.build();

plannerContext.getMetadata().addColumn(session, tableHandle.get(), column);
plannerContext.getMetadata().addColumn(session, tableHandle, column);

return immediateVoidFuture();
}
Expand Down
30 changes: 16 additions & 14 deletions core/trino-main/src/main/java/io/trino/execution/CommentTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -69,37 +70,38 @@ public ListenableFuture<Void> execute(
Session session = stateMachine.getSession();

if (statement.getType() == Comment.Type.TABLE) {
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
Optional<TableHandle> 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);
Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Feb 22, 2022

Choose a reason for hiding this comment

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

Do we need to wrap the throw with a statement existence check?

if (!statement.isTableExists()) {
    throw semanticException...
}
return immediateVoidFuture();

}

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<QualifiedName> prefix = statement.getName().getPrefix();
if (prefix.isEmpty()) {
throw semanticException(MISSING_TABLE, statement, "Table must be specified");
}

QualifiedObjectName tableName = createQualifiedObjectName(session, statement, prefix.get());
Optional<TableHandle> 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);
Copy link
Copy Markdown
Contributor Author

@findinpath findinpath Feb 22, 2022

Choose a reason for hiding this comment

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

Reminder for statement existence check.

}
TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();

String columnName = statement.getName().getSuffix();
Map<String, ColumnHandle> columnHandles = metadata.getColumnHandles(session, tableHandle.get());
Map<String, ColumnHandle> 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());
Expand Down
24 changes: 11 additions & 13 deletions core/trino-main/src/main/java/io/trino/execution/DropTableTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@
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;

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;
Expand Down Expand Up @@ -61,39 +60,38 @@ public ListenableFuture<Void> 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> 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();
}
Expand Down
60 changes: 51 additions & 9 deletions ...product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down Expand Up @@ -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);
}

Expand All @@ -340,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);
}
Expand Down Expand Up @@ -464,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()
Expand Down