Skip to content

Call purgeTable in Iceberg REST catalog#15855

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-rest-delete-table
Jan 26, 2023
Merged

Call purgeTable in Iceberg REST catalog#15855
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-rest-delete-table

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Jan 26, 2023

Description

Call purgeTable in Iceberg REST catalog

Release notes

(x) This is not user-visible or docs only and no release notes are required.

Upcoming 1.2.0 version doesn't delete data in RESTSessionCatalog.dropTable method.
@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Jan 26, 2023
@cla-bot cla-bot bot added the cla-signed label Jan 26, 2023
@ebyhr ebyhr requested a review from alexjo2144 January 26, 2023 01:34
@ebyhr ebyhr self-assigned this Jan 26, 2023
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

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

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

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

@ebyhr ebyhr merged commit 6c01038 into trinodb:master Jan 26, 2023
@ebyhr ebyhr deleted the ebi/iceberg-rest-delete-table branch January 26, 2023 22:53
@github-actions github-actions bot added this to the 407 milestone Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants