Skip to content

Add column add/drop/rename events #15225

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
electrum merged 1 commit into from
Jan 23, 2023
Merged

Add column add/drop/rename events #15225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/DisabledSystemSecurityMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,15 @@ public void tableRenamed(Session session, CatalogSchemaTableName sourceTable, Ca
@Override
public void tableDropped(Session session, CatalogSchemaTableName table) {}

@Override
public void columnCreated(Session session, CatalogSchemaTableName table, String column) {}

@Override
public void columnRenamed(Session session, CatalogSchemaTableName table, String oldColumn, String newColumn) {}

@Override
public void columnDropped(Session session, CatalogSchemaTableName table, String column) {}

private static TrinoException notSupportedException(String catalogName)
{
return new TrinoException(NOT_SUPPORTED, "Catalog does not support permission management: " + catalogName);
Expand Down
22 changes: 22 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,14 @@ public void renameColumn(Session session, TableHandle tableHandle, ColumnHandle
CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
ConnectorMetadata metadata = getMetadataForWrite(session, catalogHandle);
metadata.renameColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), source, target.toLowerCase(ENGLISH));

CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle);
ColumnMetadata sourceColumnMetadata = getColumnMetadata(session, tableHandle, source);
if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
TableMetadata tableMetadata = getTableMetadata(session, tableHandle);
CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableMetadata.getTable());
systemSecurityMetadata.columnRenamed(session, sourceTableName, sourceColumnMetadata.getName(), target);
}
}

@Override
Expand All @@ -734,14 +742,28 @@ public void addColumn(Session session, TableHandle tableHandle, ColumnMetadata c
CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
ConnectorMetadata metadata = getMetadataForWrite(session, catalogHandle);
metadata.addColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), column);

CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle);
if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
TableMetadata tableMetadata = getTableMetadata(session, tableHandle);
CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableMetadata.getTable());
systemSecurityMetadata.columnCreated(session, sourceTableName, column.getName());
}
}

@Override
public void dropColumn(Session session, TableHandle tableHandle, ColumnHandle column)
{
CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
ConnectorMetadata metadata = getMetadataForWrite(session, catalogHandle);
CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle);
metadata.dropColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), column);
if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
String columnName = getColumnMetadata(session, tableHandle, column).getName();
TableMetadata tableMetadata = getTableMetadata(session, tableHandle);
Copy link
Copy Markdown
Member

@findepi findepi Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@djsstarburst

It seems it doesn't work for some connectors.
For example, with Hive connector, metadata.dropColumn sets the SemiTransactionalHiveMetastore into EXCLUSIVE_OPERATION_BUFFERED state and then getTableMetadata fails

io.trino.testing.QueryFailedException: Unsupported combination of operations in a single transaction

	at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:122)
	at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:490)
	at io.trino.testing.QueryAssertions.assertUpdate(QueryAssertions.java:71)
	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:365)
	at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:360)
	at io.trino.testing.BaseConnectorTest.testAddAndDropColumnName(BaseConnectorTest.java:4308)
	at io.trino.testing.BaseConnectorTest.testAddAndDropColumnName(BaseConnectorTest.java:4286)
...

	Suppressed: java.lang.Exception: SQL: ALTER TABLE tcn_lowercaseh3yyp7xswo DROP COLUMN lowercase
		at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:493)
		... 29 more
Caused by: io.trino.spi.TrinoException: Unsupported combination of operations in a single transaction
	at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.checkReadable(SemiTransactionalHiveMetastore.java:2357)
	at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.getTable(SemiTransactionalHiveMetastore.java:244)
	at io.trino.plugin.hive.HiveMetadata.doGetTableMetadata(HiveMetadata.java:583)
	at io.trino.plugin.hive.HiveMetadata.getTableMetadata(HiveMetadata.java:569)
	at io.trino.plugin.hive.HiveMetadata.getTableMetadata(HiveMetadata.java:563)
	at io.trino.plugin.base.classloader.ClassLoaderSafeConnectorMetadata.getTableMetadata(ClassLoaderSafeConnectorMetadata.java:263)
...
	at io.trino.metadata.MetadataManager.getTableMetadata(MetadataManager.java:450)
	at io.trino.metadata.MetadataManager.dropColumn(MetadataManager.java:763)
	at io.trino.execution.DropColumnTask.execute(DropColumnTask.java:99)

CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableMetadata.getTable());
systemSecurityMetadata.columnDropped(session, sourceTableName, columnName);
Copy link
Copy Markdown
Member

@findepi findepi Feb 2, 2023

Choose a reason for hiding this comment

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

this may be problematic from transaction perspective.
what if we dispatch the systemSecurityMetadata.columnDropped event but the transaction doesn't get committed eventually and we end up still having the column?

credits to @losipiuk for thinking about that

Copy link
Copy Markdown
Member

@findepi findepi Feb 6, 2023

Choose a reason for hiding this comment

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

a bit related, but vice versa, a failure here will cause Trino transaction failure even if the connector applied the changes immediately (many do). This led to #12535, see #12535 (comment) for explanation

Copy link
Copy Markdown
Member

@kokosing kokosing Feb 6, 2023

Choose a reason for hiding this comment

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

Notice that problem also exists for other systemSecurityMetadata notification methods too, like alter table rename

}
}

@Override
Expand Down
15 changes: 15 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/SystemSecurityMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,19 @@ public interface SystemSecurityMetadata
* A table or view was dropped
*/
void tableDropped(Session session, CatalogSchemaTableName table);

/**
* A column was created
*/
void columnCreated(Session session, CatalogSchemaTableName table, String column);

/**
* A column was renamed
*/
void columnRenamed(Session session, CatalogSchemaTableName table, String oldColumn, String newColumn);

/**
* A column was dropped
*/
void columnDropped(Session session, CatalogSchemaTableName table, String column);
}
9 changes: 9 additions & 0 deletions testing/trino-tests/src/test/java/io/trino/security/TestingSystemSecurityMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,4 +259,13 @@ public void tableRenamed(Session session, CatalogSchemaTableName sourceTable, Ca

@Override
public void tableDropped(Session session, CatalogSchemaTableName table) {}

@Override
public void columnCreated(Session session, CatalogSchemaTableName table, String column) {}

@Override
public void columnRenamed(Session session, CatalogSchemaTableName table, String oldColumn, String newColumn) {}

@Override
public void columnDropped(Session session, CatalogSchemaTableName table, String column) {}
}