Fix correctness issue with double slash and add test for S3 and Glue metastore #17804

ebyhr · 2023-06-08T09:08:52Z

Description

Add test for Hive Thrift metastore and MinIO

Release notes

# Hive
* Fix correctness issue when location contains double slash. ({issue}`17803`)

ebyhr · 2023-06-14T09:08:29Z

.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

@@ -562,7 +563,7 @@ public synchronized void createTable(
            ConnectorSession session,
            Table table,
            PrincipalPrivileges principalPrivileges,
-            Optional<Path> currentPath,
+            Optional<Location> currentPath,


I have a concern about other usage of org.apache.hadoop.fs.Path, but hesitated to migrate everything in a PR. Let me know if I should migrate more in this PR.

we probably cannot solve all these problems at once.
we should revert 8bd9f75 to unblock the 420 release and then re-commit it behind a config toggle

if we want to maintain compatibility with Hive HMS (or Athena), then per @hashhar 's comment #17923 (review) we may need to keep normalization for ever. May require further consideration though

cc @electrum @alexjo2144 @findinpath @losipiuk

ebyhr · 2023-06-14T09:18:10Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

@@ -630,10 +639,10 @@ private static boolean isManagedTable(Table table)
        return table.getTableType().equals(MANAGED_TABLE.name());
    }

-    private static void deleteDir(HdfsContext context, HdfsEnvironment hdfsEnvironment, Path path, boolean recursive)
+    private void deleteDir(HdfsContext context, Location path)


This causes minor behavior change. For instance, we can't delete the directory having # anymore. I suppose this is acceptable and we should deny creating such a database or table.

I assume this has no real impact since for example if partition columns include # (in their name or value) the directory name contains it in URL encoded form and not raw #.

martint · 2023-06-14T17:51:34Z

Removed RELEASE-BLOCKER tag since issue is already tagged as such.

findinpath

Assuming that #17848 goes 1st, this submission LGTM.

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

findinpath · 2023-06-15T10:36:24Z

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

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class TestHiveS3AndGlueMetastoreTest


Not all these tests pass without your following commit - specifically the tests dealing with double slash within the schema location for Hive fails.

The 2nd commit doesn't run double slash test because the location is filtered in TestHiveS3AndGlueMetastoreTest.locationPatterns. Am I missing something?

findinpath · 2023-06-15T10:55:45Z

I'm looking through the productive code of Trino OSS and see that the constructor org.apache.hadoop.fs.Path#Path(java.lang.String) is still used extensively.

One such usage on which I've stumbled is:

io.trino.plugin.hive.HiveLocationService#forNewTableAsSelect

This should probably be modified.

In the test (even though currently 🟢 )

io.trino.plugin.hive.TestHiveS3AndGlueMetastoreTest#testBasicOperationsWithProvidedTableLocation for the value "s3://%s/%s//double_slash/%s", the lines should probably be modified

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveLocationService.java

Line 79 in 53b993f

if (pathExists(context, hdfsEnvironment, new Path(targetPath.toString()))) {
trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveLocationService.java

Line 66 in 53b993f

if (pathExists(context, hdfsEnvironment, new Path(targetPath.toString()))) {

There are as @ebyhr pointed out a lot of other usages which, in special constellations, will probably turn out to be other bugs related to exotic paths.
In order to be pragmatical and not end up with a giant, I'd advocate that this PR is not supposed to deal with doing all the refactorings.

findepi · 2023-06-15T19:02:05Z

@ebyhr please see test failures

ebyhr · 2023-06-16T00:25:12Z

CI failures are unrelated to this change in my opinion. I will file issues.

ci / test (plugin/trino-delta-lake, cloud-tests)
Error:    TestDeltaLakeGcsConnectorSmokeTest>BaseDeltaLakeConnectorSmokeTest.testStatsSplitPruningBasedOnSepCreatedCheckpointOnTopOfCheckpointWithJustStructStats:1552->registerTableFromResources:172 » NullPointer Cannot invoke "com.google.cloud.hadoop.repackaged.gcs.com.google.cloud.hadoop.gcsio.GoogleCloudStorage.create(com.google.cloud.hadoop.repackaged.gcs.com.google.cloud.hadoop.gcsio.StorageResourceId, com.google.cloud.hadoop.repackaged.gcs.com.google.cloud.hadoop.gcsio.CreateObjectOptions)" because "this.gcs" is null

ci / test (plugin/trino-kafka)
TestKafkaPlugin.testSslSpinup

ci / pt (default, suite-delta-lake-databricks104, )
TestDeltaLakeChangeDataFeedCompatibility.testMergeUpdateIntoTableWithCdfEnabled

ebyhr · 2023-06-16T06:54:13Z

Updated HiveLocationService. I confirmed testCreateTableWithDoubleSlash & testCtasWithDoubleSlash fail without the fix.

ebyhr · 2023-06-16T09:48:39Z

Extracted testing commit into #17930

ebyhr · 2023-06-19T06:42:20Z

Rebased on master to resolve conflicts.

findepi · 2023-06-19T09:11:10Z

TestSemiTransactionalHiveMetastore.testParallelUpdateStatisticsOperations failure is related.

https://github.com/trinodb/trino/actions/runs/5308675667/jobs/9608455959?pr=17804

ebyhr · 2023-06-19T10:53:29Z

The failure should be fixed by the new push.

findepi · 2023-06-19T15:34:06Z

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

+    @Test
+    public void testCreateTableWithDoubleSlash()
+    {
+        String schemaName = "test_create_table_with_double_slash_" + randomNameSuffix();
+        String schemaLocation = "s3://%s/%s/double_slash//test_schema".formatted(bucketName, schemaName);
+        String tableName = "test_create_table_with_double_slash_" + randomNameSuffix();
+        String schemaTableName = schemaName + "." + tableName;
+
+        // Previously, HiveLocationService replaced double slash with single slash
+        assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')");
+        String existingKey = "%s/double_slash/test_schema/%s".formatted(schemaName, tableName);
+        s3.putObject(bucketName, existingKey, "test content");
+
+        assertUpdate("CREATE TABLE " + schemaTableName + "(col_int int)");
+        assertUpdate("INSERT INTO " + schemaTableName + " VALUES 1", 1);
+        assertQuery("SELECT * FROM " + schemaTableName, "VALUES 1");
+        assertUpdate("DROP TABLE " + schemaTableName);
+        s3.deleteObject(bucketName, existingKey);
+    }
+
+    @Test
+    public void testCtasWithDoubleSlash()
+    {
+        String schemaName = "test_ctas_with_double_slash_" + randomNameSuffix();
+        String schemaLocation = "s3://%s/%s/double_slash//test_schema".formatted(bucketName, schemaName);
+        String tableName = "test_create_table_with_double_slash_" + randomNameSuffix();
+        String schemaTableName = schemaName + "." + tableName;
+
+        // Previously, HiveLocationService replaced double slash with single slash
+        assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')");
+        String existingKey = "%s/double_slash/test_schema/%s".formatted(schemaName, tableName);
+        s3.putObject(bucketName, existingKey, "test content");
+
+        assertUpdate("CREATE TABLE " + schemaTableName + " AS SELECT 1 AS col_int", 1);
+        assertQuery("SELECT * FROM " + schemaTableName, "VALUES 1");
+        assertUpdate("DROP TABLE " + schemaTableName);
+        s3.deleteObject(bucketName, existingKey);
+    }


Are these new tests needed?
When working on #17958 i was under impression that other existing tests (those with data provider) cover these cases.

These tests are needed for changes in HiveLocationService. Existing tests don't cover this scenario.

testBasicOperationsWithProvidedSchemaLocation exercises HiveLocationService
is it missing more coverage?

testBasicOperationsWithProvidedSchemaLocation doesn't cover the test when the normalized table location already exists in S3. This test puts an object on single-slash table location and then creates a table on double-slash location. HiveLocationService threw an incorrect already-exists error because the class internally replaced double-slash with single-slash.

It wasn't a silent correctness issue, so no need to fix it in this release, but it would be nice to add the test and improve it later.

agreed.
let's have explanatory comment in the test. it seems at least for me understanding its importance wasn't easy

findepi · 2023-08-01T06:23:13Z

Fixes #17803

given that issue is closed, can you please remind me which portions of this PR we still want to go ahead with?

ebyhr · 2023-10-26T01:29:18Z

Superseded by #17958

cla-bot bot added the cla-signed label Jun 8, 2023

github-actions bot added delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector tests:hive labels Jun 8, 2023

ebyhr force-pushed the ebi/hive-glue-s3-test branch 4 times, most recently from a4c6287 to 39ece22 Compare June 13, 2023 11:46

ebyhr changed the title ~~Add test for S3 and Glue metastore~~ Fix correctness issue with double slash and add test for S3 and Glue metastore Jun 14, 2023

ebyhr force-pushed the ebi/hive-glue-s3-test branch from 39ece22 to 2da154d Compare June 14, 2023 04:43

ebyhr added the RELEASE-BLOCKER label Jun 14, 2023

ebyhr force-pushed the ebi/hive-glue-s3-test branch 3 times, most recently from 1a9eab5 to a317fd8 Compare June 14, 2023 08:42

ebyhr self-assigned this Jun 14, 2023

ebyhr force-pushed the ebi/hive-glue-s3-test branch from a317fd8 to 76a7baf Compare June 14, 2023 08:51

ebyhr marked this pull request as ready for review June 14, 2023 08:52

ebyhr commented Jun 14, 2023

View reviewed changes

ebyhr force-pushed the ebi/hive-glue-s3-test branch from 76a7baf to 95e7f04 Compare June 14, 2023 09:20

ebyhr requested review from findinpath and pajaks June 14, 2023 09:48

ebyhr force-pushed the ebi/hive-glue-s3-test branch from 95e7f04 to 60ecb5a Compare June 14, 2023 13:29

martint removed the RELEASE-BLOCKER label Jun 14, 2023

ebyhr force-pushed the ebi/hive-glue-s3-test branch from 60ecb5a to 0f7bc2e Compare June 15, 2023 04:23

findinpath approved these changes Jun 15, 2023

View reviewed changes

ebyhr requested a review from findepi June 15, 2023 10:51

ebyhr force-pushed the ebi/hive-glue-s3-test branch 2 times, most recently from 49cac1e to 9d79c36 Compare June 15, 2023 12:14

ebyhr force-pushed the ebi/hive-glue-s3-test branch from d521c36 to 488e5c1 Compare June 16, 2023 06:53

findepi mentioned this pull request Jun 16, 2023

Forbid scheme://authority locations (without trailing slash) #17923

Merged

findepi mentioned this pull request Jun 16, 2023

Add test for S3 and Glue metastore #17930

Merged

ebyhr mentioned this pull request Jun 19, 2023

Revert "Remove usages of Hadoop Path for Hive LocationService" #17947

Closed

ebyhr force-pushed the ebi/hive-glue-s3-test branch from 488e5c1 to fececf7 Compare June 19, 2023 06:42

Fix correctness issue of double slash in Hive connector

8cb4bd1

ebyhr force-pushed the ebi/hive-glue-s3-test branch from fececf7 to 8cb4bd1 Compare June 19, 2023 10:46

findepi reviewed Jun 19, 2023

View reviewed changes

electrum mentioned this pull request Jul 25, 2023

Decouple Trino from Hadoop and Hive codebases #15921

Closed

ebyhr closed this Oct 26, 2023

ebyhr deleted the ebi/hive-glue-s3-test branch October 26, 2023 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix correctness issue with double slash and add test for S3 and Glue metastore #17804

Fix correctness issue with double slash and add test for S3 and Glue metastore #17804

ebyhr commented Jun 8, 2023 •

edited

Loading

ebyhr Jun 14, 2023 •

edited

Loading

findepi Jun 16, 2023

findepi Jun 16, 2023

ebyhr Jun 14, 2023

hashhar Jun 16, 2023

martint commented Jun 14, 2023

findinpath left a comment

findinpath Jun 15, 2023

ebyhr Jun 15, 2023 •

edited

Loading

findinpath commented Jun 15, 2023 •

edited

Loading

findepi commented Jun 15, 2023

ebyhr commented Jun 16, 2023 •

edited

Loading

ebyhr commented Jun 16, 2023

ebyhr commented Jun 16, 2023

ebyhr commented Jun 19, 2023

findepi commented Jun 19, 2023 •

edited

Loading

ebyhr commented Jun 19, 2023

findepi Jun 19, 2023

ebyhr Jun 19, 2023

findepi Jun 21, 2023

ebyhr Jun 22, 2023

findepi Jun 22, 2023

findepi commented Aug 1, 2023

ebyhr commented Oct 26, 2023

Fix correctness issue with double slash and add test for S3 and Glue metastore #17804

Fix correctness issue with double slash and add test for S3 and Glue metastore #17804

Conversation

ebyhr commented Jun 8, 2023 • edited Loading

Description

Release notes

ebyhr Jun 14, 2023 • edited Loading

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

martint commented Jun 14, 2023

findinpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

findinpath commented Jun 15, 2023 • edited Loading

findepi commented Jun 15, 2023

ebyhr commented Jun 16, 2023 • edited Loading

ebyhr commented Jun 16, 2023

ebyhr commented Jun 16, 2023

ebyhr commented Jun 19, 2023

findepi commented Jun 19, 2023 • edited Loading

ebyhr commented Jun 19, 2023

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

findepi commented Aug 1, 2023

ebyhr commented Oct 26, 2023

ebyhr commented Jun 8, 2023 •

edited

Loading

ebyhr Jun 14, 2023 •

edited

Loading

ebyhr Jun 15, 2023 •

edited

Loading

findinpath commented Jun 15, 2023 •

edited

Loading

ebyhr commented Jun 16, 2023 •

edited

Loading

findepi commented Jun 19, 2023 •

edited

Loading