Skip to content

Call purgeTable in Iceberg REST catalog #15855

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

Call purgeTable in Iceberg REST catalog #15855

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
2 changes: 1 addition & 1 deletion ...in/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public void registerTable(ConnectorSession session, SchemaTableName tableName, S
@Override
public void dropTable(ConnectorSession session, SchemaTableName schemaTableName)
{
if (!restSessionCatalog.dropTable(convert(session), toIdentifier(schemaTableName))) {
if (!restSessionCatalog.purgeTable(convert(session), toIdentifier(schemaTableName))) {
Copy link
Copy Markdown
Contributor

@danielcweeks danielcweeks Jan 26, 2023

Choose a reason for hiding this comment

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

@ebyhr Rather than change the current behavior, can we make this a configuration property? There are many situations where you don't want drop to destroy data and I'd rather be explicit about the intent rather than have systems ignore the request (like is done in many cases with Hive Metastore).

Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We've been moving towards solving that situation with an unregister_table procedure: #15807 and #15853

DROP TABLE is an unrecoverable operation across all the other Trino connectors, so I think that a separate procedure like this will be clearer to users than a config property that gets set at cluster creation.

Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 Jan 26, 2023

Choose a reason for hiding this comment

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

Hopefully the symmetrical naming with the register_table procedure will make it more intuitive too

throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to drop table: %s", schemaTableName));
}
}
Expand Down
15 changes: 15 additions & 0 deletions plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5045,6 +5045,21 @@ public void testDroppingIcebergAndCreatingANewTableWithTheSameNameShouldBePossib
dropTable("test_iceberg_recreate");
}

@Test
public void testDropTableDeleteData()
{
String tableName = "test_drop_table_delete_data" + randomNameSuffix();
assertUpdate("CREATE TABLE " + tableName + " (a_int) AS VALUES (1)", 1);
String tableLocation = getTableLocation(tableName);
assertUpdate("DROP TABLE " + tableName);

// Create a new table with the same location to verify the data was deleted in the above DROP TABLE
assertUpdate("CREATE TABLE " + tableName + "(a_int INTEGER) WITH (location = '" + tableLocation + "')");
assertQueryReturnsEmptyResult("SELECT * FROM " + tableName);

assertUpdate("DROP TABLE " + tableName);
}

@Test
public void testPathHiddenColumn()
{
Expand Down