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

plugin/trino-hive-hadoop2/bin/run_hive_s3_tests.sh

@@ -92,6 +92,7 @@ set +e
     -Dhive.hadoop2.metastoreHost=localhost \
     -Dhive.hadoop2.metastorePort=9083 \
     -Dhive.hadoop2.databaseName=default \
+    -Dhive.hadoop2.s3.endpoint="${S3_BUCKET_ENDPOINT}" \
     -Dhive.hadoop2.s3.awsAccessKey="${AWS_ACCESS_KEY_ID}" \
     -Dhive.hadoop2.s3.awsSecretKey="${AWS_SECRET_ACCESS_KEY}" \
     -Dhive.hadoop2.s3.writableBucket="${S3_BUCKET}" \

plugin/trino-hive-hadoop2/pom.xml

@@ -146,6 +146,12 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>io.trino</groupId>
+            <artifactId>trino-testing-containers</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.airlift</groupId>
             <artifactId>testing</artifactId>
@@ -180,6 +186,7 @@
                             <excludes>
                                 <exclude>**/TestHive.java</exclude>
                                 <exclude>**/TestHiveAlluxioMetastore.java</exclude>
+                                <exclude>**/TestHiveThriftMetastoreWithS3.java</exclude>
                                 <exclude>**/TestHiveFileSystemS3.java</exclude>
                                 <exclude>**/TestHiveFileSystemS3SelectPushdown.java</exclude>
                                 <exclude>**/TestHiveFileSystemS3SelectJsonPushdown.java</exclude>
@@ -220,6 +227,7 @@
                         <artifactId>maven-surefire-plugin</artifactId>
                         <configuration>
                             <includes>
+                                <include>**/TestHiveThriftMetastoreWithS3.java</include>
                                 <include>**/TestHiveFileSystemS3.java</include>
                                 <include>**/TestHiveFileSystemS3SelectPushdown.java</include>
                                 <include>**/TestHiveFileSystemS3SelectPushdownWithSplits.java</include>

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

@@ -0,0 +1,126 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.hive;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.io.Resources;
+import io.trino.plugin.hive.containers.HiveHadoop;
+import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreConfig;
+import io.trino.plugin.hive.s3.S3HiveQueryRunner;
+import io.trino.testing.AbstractTestQueryFramework;
+import io.trino.testing.QueryRunner;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Parameters;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermissions;
+
+import static io.trino.testing.sql.TestTable.randomTableSuffix;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Objects.requireNonNull;
+
+public class TestHiveThriftMetastoreWithS3
+        extends AbstractTestQueryFramework
+{
+    private final String awsS3endpoint;
+    private final String awsAccessKey;
+    private final String awsSecretKey;
+    private final String writableBucket;
+    private final String schemaName;
+    private final Path hadoopCoreSiteXmlTempFile;
+
+    @Parameters({
+            "hive.hadoop2.s3.endpoint",
+            "hive.hadoop2.s3.awsAccessKey",
+            "hive.hadoop2.s3.awsSecretKey",
+            "hive.hadoop2.s3.writableBucket",
+    })
+    public TestHiveThriftMetastoreWithS3(
+            String awsS3endpoint,
+            String awsAccessKey,
+            String awsSecretKey,
+            String writableBucket)
+            throws IOException
+    {
+        this.awsS3endpoint = requireNonNull(awsS3endpoint, "awsS3endpoint is null");
+        this.awsAccessKey = requireNonNull(awsAccessKey, "awsAccessKey is null");
+        this.awsSecretKey = requireNonNull(awsSecretKey, "awsSecretKey is null");
+        this.writableBucket = requireNonNull(writableBucket, "writableBucket is null");
+        this.schemaName = "test_thrift_s3_" + randomTableSuffix();
+
+        String coreSiteXmlContent = Resources.toString(Resources.getResource("s3/hive-core-site.template.xml"), UTF_8)
+                .replace("%S3_BUCKET_ENDPOINT%", awsS3endpoint)
+                .replace("%AWS_ACCESS_KEY_ID%", awsAccessKey)
+                .replace("%AWS_SECRET_ACCESS_KEY%", awsSecretKey);
+
+        hadoopCoreSiteXmlTempFile = Files.createTempFile("core-site", ".xml", PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-r--r--")));
+        hadoopCoreSiteXmlTempFile.toFile().deleteOnExit();
+        Files.writeString(hadoopCoreSiteXmlTempFile, coreSiteXmlContent);
+    }
+
+    @Override
+    protected QueryRunner createQueryRunner()
+            throws Exception
+    {
+        HiveHadoop hiveHadoop = HiveHadoop.builder()
+                .withFilesToMount(ImmutableMap.of("/etc/hadoop/conf/core-site.xml", hadoopCoreSiteXmlTempFile.normalize().toAbsolutePath().toString()))
+                .build();
+        hiveHadoop.start();
+
+        return S3HiveQueryRunner.builder()
+                .setHiveMetastoreEndpoint(hiveHadoop.getHiveMetastoreEndpoint())
+                .setS3Endpoint(awsS3endpoint)
+                .setS3AccessKey(awsAccessKey)
+                .setS3SecretKey(awsSecretKey)
+                .setBucketName(writableBucket)
+                .setPopulateTpchData(false)
+                .setThriftMetastoreConfig(new ThriftMetastoreConfig().setDeleteFilesOnDrop(true))
+                .build();
+    }
+
+    @BeforeClass
+    public void setUp()
+    {
+        String schemaLocation = "s3a://%s/%s".formatted(writableBucket, schemaName);
+        assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')");
+    }
+
+    @AfterClass(alwaysRun = true)
+    public void tearDown()
+    {
+        assertUpdate("DROP SCHEMA IF EXISTS " + schemaName);
+    }
+
+    @Test
+    public void testRecreateTable()
+    {
+        String tableName = "%s.test_recreate_table_%s".formatted(schemaName, randomTableSuffix());
+        assertUpdate("CREATE TABLE " + tableName + "(col int)");
+        try {
+            assertUpdate("INSERT INTO " + tableName + " VALUES (1)", 1);
+            assertUpdate("DROP TABLE " + tableName);
+
+            // 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)");
+        }
+        finally {
+            assertUpdate("DROP TABLE IF EXISTS " + tableName);
+        }
+    }
+}

plugin/trino-hive-hadoop2/src/test/resources/s3/hive-core-site.template.xml

@@ -0,0 +1,43 @@
+<?xml version="1.0"?>
+<configuration>
+    <property>
+        <name>fs.defaultFS</name>
+        <value>hdfs://hadoop-master:9000</value>
+    </property>
+
+    <property>
+        <name>fs.s3a.endpoint</name>
+        <value>%S3_BUCKET_ENDPOINT%</value>
+    </property>
+
+    <property>
+        <name>fs.s3.awsAccessKeyId</name>
+        <value>%AWS_ACCESS_KEY_ID%</value>
+    </property>
+
+    <property>
+        <name>fs.s3.awsSecretAccessKey</name>
+        <value>%AWS_SECRET_ACCESS_KEY%</value>
+    </property>
+
+    <property>
+        <name>fs.s3a.access.key</name>
+        <value>%AWS_ACCESS_KEY_ID%</value>
+    </property>
+
+    <property>
+        <name>fs.s3a.secret.key</name>
+        <value>%AWS_SECRET_ACCESS_KEY%</value>
+    </property>
+
+    <!-- Hive impersonation -->
+    <property>
+        <name>hadoop.proxyuser.hive.hosts</name>
+        <value>*</value>
+    </property>
+
+    <property>
+        <name>hadoop.proxyuser.hive.groups</name>
+        <value>*</value>
+    </property>
+</configuration>

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

@@ -117,6 +117,7 @@ public static class Builder<SELF extends Builder<?>>
         private Optional<DirectoryLister> directoryLister = Optional.empty();
         private boolean tpcdsCatalogEnabled;
         private String security = SQL_STANDARD;
+        private boolean populateTpchData = true;
         private ColumnNaming tpchColumnNaming = SIMPLIFIED;
         private DecimalTypeMapping tpchDecimalTypeMapping = DOUBLE;
 
@@ -197,6 +198,12 @@ public SELF setSecurity(String security)
             return self();
         }
 
+        public SELF setPopulateTpchData(boolean populateTpchData)
+        {
+            this.populateTpchData = populateTpchData;
+            return self();
+        }
+
         public SELF setTpchColumnNaming(ColumnNaming tpchColumnNaming)
         {
             this.tpchColumnNaming = requireNonNull(tpchColumnNaming, "tpchColumnNaming is null");
@@ -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) {
+                    populateData(queryRunner, metastore);
+                }
 
                 return queryRunner;
             }

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

@@ -21,6 +21,7 @@
 import io.trino.plugin.hive.containers.HiveMinioDataLake;
 import io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore;
 import io.trino.plugin.hive.metastore.thrift.TestingMetastoreLocator;
+import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreConfig;
 import io.trino.testing.DistributedQueryRunner;
 import io.trino.tpch.TpchTable;
 
@@ -85,6 +86,7 @@ public static class Builder
     {
         private HostAndPort hiveMetastoreEndpoint;
         private Duration thriftMetastoreTimeout = TestingMetastoreLocator.TIMEOUT;
+        private ThriftMetastoreConfig thriftMetastoreConfig = new ThriftMetastoreConfig();
         private String s3Endpoint;
         private String s3AccessKey;
         private String s3SecretKey;
@@ -102,6 +104,12 @@ public Builder setThriftMetastoreTimeout(Duration thriftMetastoreTimeout)
             return this;
         }
 
+        public Builder setThriftMetastoreConfig(ThriftMetastoreConfig thriftMetastoreConfig)
+        {
+            this.thriftMetastoreConfig = requireNonNull(thriftMetastoreConfig, "thriftMetastoreConfig is null");
+            return this;
+        }
+
         public Builder setS3Endpoint(String s3Endpoint)
         {
             this.s3Endpoint = requireNonNull(s3Endpoint, "s3Endpoint is null");
@@ -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://"),
+                    "Expected S3 hostname or http URI for S3 endpoint; got %s", s3Endpoint);
 
             addHiveProperty("hive.s3.endpoint", s3Endpoint);
             addHiveProperty("hive.s3.aws-access-key", s3AccessKey);
@@ -145,6 +155,7 @@ public DistributedQueryRunner build()
             setMetastore(distributedQueryRunner -> new BridgingHiveMetastore(
                     testingThriftHiveMetastoreBuilder()
                             .metastoreClient(hiveMetastoreEndpoint, thriftMetastoreTimeout)
+                            .thriftMetastoreConfig(thriftMetastoreConfig)
                             .build()));
             setInitialSchemasLocationBase("s3a://" + bucketName); // cannot use s3:// as Hive metastore is not configured to accept it
             return super.build();