Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions #13822

ebyhr · 2022-08-24T09:41:27Z

Description

Respect hive.metastore.thrift.delete-files-on-drop config property for dropping partitions
Fixes #13545 Fixes #13821

Documentation

(x) Sufficient documentation is included in this PR.

Release notes

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

# Hive
* Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions. ({issue}`13545`)

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

alexjo2144 · 2022-08-25T17:09:23Z

The Hive connector doc needs an update if we're going to do this. Specifically:

system.unregister_partition(schema_name, table_name, partition_columns, partition_values)

Unregisters given, existing partition in the metastore for the specified table. The partition data is not deleted.

ebyhr · 2022-09-12T03:17:30Z

I don't think we need to update unregister_partition docs since deleteData argument is false. I will add a test for the procedure though.

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/procedure/UnregisterPartitionProcedure.java

Lines 107 to 112 in bcfe05f

    
           metastore.dropPartition( 
        
                   session, 
        
                   table.getDatabaseName(), 
        
                   table.getTableName(), 
        
                   partition.getValues(), 
        
                   false);

ebyhr · 2022-09-13T04:53:54Z

plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHiveThriftMetastoreWithS3.java

+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+public class TestHiveThriftMetastoreWithS3


Using normal test environment because somehow I can't reproduce the issue on product test environment.

alexjo2144 · 2022-09-27T16:59:21Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

                            client.dropPartition(databaseName, tableName, parts, deleteData);
+                            String partitionLocation = partition.getSd().getLocation();


Should we wrap the file deletion in a separate try/catch?

I'm just thinking, if the partition is dropped from the metastore but the file system doesn't allow us to delete the files for some reason, the query should still succeed right?

I feel users should disable it in such cases. This feature can be controlled by config property, so exception looks better in my opinion.

I think it depends on what the behavior is for a following select statement. If I run a drop partition and it fails, the partition should still show up in the next query. If it doesn't show up, the drop should succeed. Does that make sense?

I was missing that subsequent deleteDirRecursive already has try-catch block in the method. Do you think it still requires additional change?

You're totally right. This is fine

findinpath · 2022-09-28T10:44:15Z

There are several CI failures.

findepi · 2022-09-28T10:51:11Z

There are several CI failures.

Added empty commit to retrigger the build after #14323 merged.

ebyhr · 2022-10-05T22:35:39Z

CI hit #14441

findepi · 2022-10-07T09:57:00Z

plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHiveThriftMetastoreWithS3.java

+                .setBucketName(writableBucket)
+                .setPopulateTpchData(false)
+                .setThriftMetastoreConfig(new ThriftMetastoreConfig().setDeleteFilesOnDrop(true))
+                .setHiveProperties(ImmutableMap.of("hive.allow-register-partition-procedure", "true"))


It's not redundant. ThriftMetastoreConfig.setDeleteFilesOnDrop is equals to hive.metastore.thrift.delete-files-on-drop not hive.allow-register-partition-procedure.

findepi · 2022-10-07T09:59:29Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveQueryRunner.java

@@ -255,7 +262,9 @@ public DistributedQueryRunner build()
                queryRunner.createCatalog(HIVE_CATALOG, "hive", hiveProperties);
                queryRunner.createCatalog(HIVE_BUCKETED_CATALOG, "hive", hiveBucketedProperties);

-                populateData(queryRunner, metastore);
+                if (populateTpchData) {


else validate initialTables is empty, since it's ignored otherwise

however, a better way for this would be to call the parameter createTpchSchemas
then createTpchSchemas(false) would skip schema cretion and initialTables(empty) (default) would skip tables creation

Renamed to createTpchSchemas .

findepi · 2022-10-07T10:01:33Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/S3HiveQueryRunner.java

@@ -136,7 +144,9 @@ public DistributedQueryRunner build()
            requireNonNull(s3SecretKey, "s3SecretKey is null");
            requireNonNull(bucketName, "bucketName is null");
            String lowerCaseS3Endpoint = s3Endpoint.toLowerCase(Locale.ENGLISH);
-            checkArgument(lowerCaseS3Endpoint.startsWith("http://") || lowerCaseS3Endpoint.startsWith("https://"), "Expected http URI for S3 endpoint; got %s", s3Endpoint);
+            checkArgument(
+                    lowerCaseS3Endpoint.matches("s3.*\\.amazonaws\\.com") || lowerCaseS3Endpoint.startsWith("http://") || lowerCaseS3Endpoint.startsWith("https://"),


are we using s3.*.amazonaws.com without https://? should we add it explicitly? in secrets?

Pushed another commit to add the prefix.

homar · 2022-10-07T20:13:38Z

plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHiveThriftMetastoreWithS3.java

+
+            // Recreating a table throws "Target directory for table 'xxx' already exists" in real S3 (not MinIO)
+            // when 'hive.metastore.thrift.delete-files-on-drop' config property is false
+            assertUpdate("CREATE TABLE " + tableName + "(col int)");


Maybe it would make sense to check that the table in fact is empty?

Included additional assertions.

ebyhr · 2022-10-11T09:34:55Z

Addressed comments.

findepi · 2022-10-12T06:54:54Z

LGTM, but the build is red

ebyhr · 2022-10-12T06:59:51Z

Failed jobs succeeded in #14557 (within the repo). I will take a look these failures and file issues if needed.

ebyhr · 2022-10-12T07:51:57Z

CI hit

sumanshusamarora · 2024-03-11T23:36:52Z

@ebyhr Is there a similar property available for glue metastore. Upon drop table command, the table meta information is deleted from glue but the underlying data remains in azure. Anything you can suggest to allow dropping of data?

cla-bot bot added the cla-signed label Aug 24, 2022

github-actions bot added docs tests:hive labels Aug 24, 2022

ebyhr commented Aug 24, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved

ebyhr requested review from findepi, findinpath, homar and alexjo2144 August 24, 2022 23:21

findinpath approved these changes Aug 25, 2022

View reviewed changes

homar approved these changes Aug 25, 2022

View reviewed changes

findepi reviewed Aug 25, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved

ebyhr mentioned this pull request Sep 12, 2022

Add test for hive.metastore.thrift.delete-files-on-drop config property #13892

Closed

ebyhr force-pushed the ebi/hive-delete-files-on-drop branch from f7ff00b to 577addc Compare September 13, 2022 04:48

ebyhr commented Sep 13, 2022

View reviewed changes

ebyhr force-pushed the ebi/hive-delete-files-on-drop branch from 577addc to eceb235 Compare September 27, 2022 12:42

alexjo2144 reviewed Sep 27, 2022

View reviewed changes

ebyhr force-pushed the ebi/hive-delete-files-on-drop branch from 6af2c70 to db0758b Compare October 5, 2022 09:31

ebyhr requested review from findepi and alexjo2144 October 5, 2022 22:35

findinpath approved these changes Oct 6, 2022

View reviewed changes

findepi reviewed Oct 7, 2022

View reviewed changes

homar reviewed Oct 7, 2022

View reviewed changes

homar approved these changes Oct 7, 2022

View reviewed changes

ebyhr added 2 commits October 11, 2022 12:38

Set https scheme explicitly as S3_BUCKET_ENDPOINT

75cd468

Add test for hive.metastore.thrift.delete-files-on-drop

0701c9f

Respect Thrift delete-files-on-drop for dropping partitions

3407144

ebyhr force-pushed the ebi/hive-delete-files-on-drop branch from db0758b to 3407144 Compare October 11, 2022 04:45

ebyhr mentioned this pull request Oct 11, 2022

Test: Respect hive.metastore.thrift.delete-files-on-drop config property for dropping partitions #14557

Closed

ebyhr requested a review from findepi October 12, 2022 12:13

ebyhr merged commit a87fc63 into trinodb:master Oct 12, 2022

ebyhr deleted the ebi/hive-delete-files-on-drop branch October 12, 2022 12:33

ebyhr mentioned this pull request Oct 12, 2022

Release notes for 400 #14509

Closed

github-actions bot added this to the 400 milestone Oct 12, 2022

colebow mentioned this pull request Oct 12, 2022

Add Trino 400 release notes #14556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions #13822

Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions #13822

ebyhr commented Aug 24, 2022 •

edited

Loading

alexjo2144 commented Aug 25, 2022

ebyhr commented Sep 12, 2022

ebyhr Sep 13, 2022

alexjo2144 Sep 27, 2022

ebyhr Sep 27, 2022

alexjo2144 Sep 28, 2022

ebyhr Oct 5, 2022

alexjo2144 Oct 5, 2022

findinpath commented Sep 28, 2022

findepi commented Sep 28, 2022

ebyhr commented Oct 5, 2022

findepi Oct 7, 2022

ebyhr Oct 11, 2022

findepi Oct 7, 2022

ebyhr Oct 11, 2022

findepi Oct 7, 2022

ebyhr Oct 11, 2022

homar Oct 7, 2022

ebyhr Oct 11, 2022

ebyhr commented Oct 11, 2022

findepi commented Oct 12, 2022

ebyhr commented Oct 12, 2022 •

edited

Loading

ebyhr commented Oct 12, 2022

sumanshusamarora commented Mar 11, 2024

		client.dropPartition(databaseName, tableName, parts, deleteData);
		String partitionLocation = partition.getSd().getLocation();

Respect hive.metastore.thrift.delete-files-on-drop config property for dropping partitions #13822

Respect hive.metastore.thrift.delete-files-on-drop config property for dropping partitions #13822

Conversation

ebyhr commented Aug 24, 2022 • edited Loading

Description

Documentation

Release notes

alexjo2144 commented Aug 25, 2022

ebyhr commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Sep 28, 2022

findepi commented Sep 28, 2022

ebyhr commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Oct 11, 2022

findepi commented Oct 12, 2022

ebyhr commented Oct 12, 2022 • edited Loading

ebyhr commented Oct 12, 2022

sumanshusamarora commented Mar 11, 2024

Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions #13822

Respect `hive.metastore.thrift.delete-files-on-drop` config property for dropping partitions #13822

ebyhr commented Aug 24, 2022 •

edited

Loading

ebyhr commented Oct 12, 2022 •

edited

Loading