Delete Iceberg table data on DROP

[findinpath]

Description

Delegate the responsibility of deleting the data files
of the table to Iceberg because the data files
of the Iceberg table may be located in different locations.

The DROP statement deletes the Iceberg table data irrespective
of the fact that the location parameter has been specified
explicitly on the table, or it was implicitly derived from
the schema location.

It is important to note that the DROP functionality can be currently
performed only for Iceberg tables that do not contain path overrides
(write.object-storage.path, write.folder-storage.path, write.metadata.path).

General information

Is this change a fix, improvement, new feature, refactoring, or other?

This is a fix.
Previously the data of the Iceberg table was not removed by HMS because Iceberg tables are marked implicitly as EXTERNAL, reason why the data of the table is not deleted by HMS on DROP.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

This change relates solely to the Iceberg connector.

How would you describe this change to a non-technical end user or system administrator?

This feature brings the ability to delete the data files while performing a DROP statements.

Related issues, pull requests, and links

Fixes #5616
Related PR: #6063
Depends on PR: #11032

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Delete data files on DROP for managed tables. ({issue}`5616`)

[findinpath]

@mosabua can you please indicate whether we should document explicitly in the documentation of the connector the DROP functionality?
Currently there are no details provided on how DROP is supposed to work for managed/external Iceberg tables https://trino.io/docs/current/connector/iceberg.html

I had a look over https://docs.starburst.io/latest/connector/starburst-delta-lake.html which implements as well this concept and haven't seen any reference there for DROP either.

[jirassimok]

I think the first commit is a pretty good change, though I'd reword it to clarify that it only affects FileHiveMetastore. Something like "allow alternate table data locations in FileHiveMetastore."

I didn't look at the second commit in detail, but I think the commit message should note that it's basically the same way HiveMetadata handles the same information.

[alexjo2144]

I was just taking a look through the Iceberg API utilities, did you see https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L78 ?

Looks like it may be a safer way of dropping table files

[findinpath]

I was just taking a look through the Iceberg API utilities, did you see https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L78 ?

@alexjo2144 yes I did see also this utility function and to be honest, I was planning to delegate the deletion logic to this method. However, in the current state of Trino, there are tables being dropped only when they don't have any overridden location properties. So, in the current stage of the Iceberg connector implementation, the deletion of the table data can be still delegated to the Hive Metastore.

[alexjo2144]

I think the main advantage to using that utility instead of delegating the delete to Hive is consistency between tables made my Spark and tables made by Trino.

If I follow the same steps:

• Create a schema with a location
• Create a table using the default location

In either Spark or Trino, and then drop the tables from Trino, I would expect it to do the same thing in either case, but that's not true here since we're relying on the managed/external flag being set on table creation and Spark will always set it to be external.

Does that make sense?

[findinpath]

@alexjo2144 this goes rather in the direction of interpreting all tables as EXTERNAL (the current status quo before this PR).
If I understand correctly, Trino would take then the responsibility of removing all the Iceberg tables on DROP TABLE.

[jirassimok]

interpreting all tables as EXTERNAL (the current status quo before this PR)

I don't think this is the case. I believe a table is only supposed to be considered external if it has the external_location property.

[alexjo2144]

I don't think this is the case. I believe a table is only supposed to be considered external if it has the external_location property.

In the Hive connector, yes, but Iceberg does not really have a well defined managed/external definition yet.

this goes rather in the direction of interpreting all tables as EXTERNAL (the current status quo before this PR).
If I understand correctly, Trino would take then the responsibility of removing all the Iceberg tables on DROP TABLE.

That's correct, personally I think this would be a better approach.

[findinpath]

I went ahead and looked through the iceberg's implementation for spark TableCatalog and see in org.apache.iceberg.hive.HiveCatalog#dropTable
https://github.com/apache/iceberg/blob/e1c801653db05f5ac2fbaecf36be4320806c27b2/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L163-L172

    try {
      clients.run(client -> {
        client.dropTable(database, identifier.name(),
            false /* do not delete data */,
            false /* throw NoSuchObjectException if the table doesn't exist */);
        return null;
      });

      if (purge && lastMetadata != null) {
        CatalogUtil.dropTableData(ops.io(), lastMetadata);
      }

As it can be seen, Spark does delegate to CatalogUtil the responsibility of deleting the table data in a file-by-file fashion.

[lzeiming]

I think the main advantage to using that utility instead of delegating the delete to Hive is consistency between tables made my Spark and tables made by Trino.

If I follow the same steps:

• Create a schema with a location
• Create a table using the default location

In either Spark or Trino, and then drop the tables from Trino, I would expect it to do the same thing in either case, but that's not true here since we're relying on the managed/external flag being set on table creation and Spark will always set it to be external.

Does that make sense?

I think so. Renaming a managed table may cause Hive metastore to do table location path rename on the filesystem. I think it is problematic for iceberg

[findinpath]

Rebasing the PR on master to make use of the update to iceberg library to the version 0.13.1.