Avoid creating Hive tables with double slashed location #17958

findepi · 2023-06-19T15:28:45Z

Hive connector does not support table locations containing double slashes. On S3 this leads to correctness issues (e.g. INSERT works, but SELECT does not find any data).

This commit

restores normalization of implicit table location during CREATE TABLE. There used to be such normalization until 8bd9f75.
rejects explicit table locations containing double slash during CREATE TABLE .. WITH (external_location = ...). Before 8bd9f75 there used to be normalization also during this flow, but rejecting such unsupported locations is deemed more correct.

Alternative to #17947
Stop gap solution until we fix all problems related to double slashes in Hive connector (#17803).

After merging this, #17803 is no longer a release-blocker.

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveWriteUtils.java

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

losipiuk

LGTM. @findinpath @alexjo2144 PTAL.

findinpath · 2023-06-20T06:13:13Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveWriteUtils.java

-        return Location.of(location).appendPath(escapeTableName(tableName));
+        // Note: this results in a "normalized location", e.g. not containing double slashes.
+        // TODO (https://github.com/trinodb/trino/issues/17803): We need to use normalized location until all relevant Hive connector components are migrated off Hadoop Path.
+        return Location.of(databasePath.toString()).appendPath(escapeTableName(tableName));


trino> create schema hive.doubleslashes with (location='s3://trino-test/double_slash//location'); CREATE SCHEMA trino> show create schema hive.doubleslashes; Create Schema ---------------------------------------------------------------- CREATE SCHEMA hive.doubleslashes WITH ( location = 's3://trino-test/double_slash//location' ) (1 row) trino> create table hive.doubleslashes.test1 (c integer); CREATE TABLE trino> insert into hive.doubleslashes.test1 values 1,2,3; INSERT: 3 rows sele Query 20230620_060740_00002_8c4bz, FINISHED, 1 node http://localhost:8080/ui/query.html?20230620_060740_00002_8c4bz Splits: 14 total, 14 done (100.00%) CPU Time: 0.3s total, 0 rows/s, 0B/s, 28% active Per Node: 0.1 parallelism, 0 rows/s, 0B/s Parallelism: 0.1 Peak Memory: 38.2KB 3.11 [0 rows, 0B] [0 rows/s, 0B/s] trino> select "$path" from hive.doubleslashes.test1; $path ---------------------------------------------------------------------------------------------------------------------- s3://trino-test/double_slash/location/test1/20230620_060740_00002_8c4bz_e91a2ac5-4bc1-40ba-ab3f-2145eaa6bf97 s3://trino-test/double_slash/location/test1/20230620_060740_00002_8c4bz_e91a2ac5-4bc1-40ba-ab3f-2145eaa6bf97 s3://trino-test/double_slash/location/test1/20230620_060740_00002_8c4bz_e91a2ac5-4bc1-40ba-ab3f-2145eaa6bf97 (3 rows)

Note that this strategy may eventually cause issues with managed tables which get created into schemas having a double-slashed location.

schema location: s3://trino-test/double_slash//location
table location: s3://trino-test/double_slash/location/test1

schema location: s3://trino-test/double_slash//location
table location: s3://trino-test/double_slash/location/test1

well, that's the whole point of this PR

thanks for calling this out!

findepi · 2023-06-20T08:25:12Z

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

+        // TODO (https://github.com/trinodb/trino/issues/17803) We cannot accept locations with double slash until all relevant Hive connector components are migrated off Hadoop Path.
+        // Hadoop Path "normalizes location", e.g.:
+        //  - removes double slashes (such locations are rejected),
+        //  - removes trailing slash (here ignored; foo/bar and foo/bar/ are treated as equivalent, and rejecting locations with trailing slash could pose UX issues)
+        //  - replaces file:///<local-path> with file:/<local-path> (here ignored).
+        if (validated.path().replaceFirst("/+$", "").contains("//")) {
+            throw new TrinoException(INVALID_TABLE_PROPERTY, "Unsupported location that cannot be internally represented: " + location);
+        }


Rewrote the logic to accept locations such as file:///tmp/6383092576615971655, file:///tmp/6383092576615971655/, s3://bucket/foo/.

as found in #17964 , two trailing slashes are not OK either.
i will not add a test case for that, but will block such locations.
test case coming in the other PR

ebyhr

Looks good except for CI failure:

Error:  Failures: 
Error:    TestHiveS3AndGlueMetastoreTest.testBasicOperationsWithProvidedTableLocation:137 
expected: "s3://trino-ci-test/test_glue_s3_icyjziw7ri/trailing_slash/test_basic_operations_kjxzwqj9yr"
 but was: "s3://trino-ci-test/test_glue_s3_icyjziw7ri/trailing_slash/test_basic_operations_kjxzwqj9yr/"
Error:    TestHiveS3AndGlueMetastoreTest.testBasicOperationsWithProvidedTableLocation:137 
expected: "s3://trino-ci-test/test_glue_s3_icyjziw7ri/trailing_slash/test_basic_operations_tuoqpbqkww"
 but was: "s3://trino-ci-test/test_glue_s3_icyjziw7ri/trailing_slash/test_basic_operations_tuoqpbqkww/"

Hive connector does not support table locations containing double slashes. On S3 this leads to correctness issues (e.g. INSERT works, but SELECT does not find any data). This commit - restores normalization of implicit table location during CREATE TABLE. There used to be such normalization until 8bd9f75. - rejects explicit table locations containing double slash during `CREATE TABLE .. WITH (external_location = ...)`. Before 8bd9f75 there used to be normalization also during this flow, but rejecting such unsupported locations is deemed more correct.

colebow · 2023-06-21T21:57:37Z

Does this need release notes? @findepi

findepi · 2023-06-22T08:56:27Z

yes. it fixes #17803

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

findepi force-pushed the findepi/hive-restore-normalization branch from 69692fc to d0e7c99 Compare June 19, 2023 15:30

findepi commented Jun 19, 2023

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved

plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveWriteUtils.java Show resolved Hide resolved

findepi requested review from electrum, martint, ebyhr, findinpath and pajaks June 19, 2023 15:30

This was referenced Jun 19, 2023

Correctness issue with double slash location on Glue & S3 in Hive connector #17803

Closed

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

Closed

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

Closed

findepi requested review from losipiuk and alexjo2144 June 19, 2023 15:36

losipiuk reviewed Jun 19, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java Outdated Show resolved Hide resolved

losipiuk reviewed Jun 19, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java Outdated Show resolved Hide resolved

losipiuk reviewed Jun 19, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java Outdated Show resolved Hide resolved

losipiuk approved these changes Jun 19, 2023

View reviewed changes

github-actions bot added hive Hive connector tests:hive labels Jun 19, 2023

findinpath reviewed Jun 20, 2023

View reviewed changes

findepi force-pushed the findepi/hive-restore-normalization branch from d0e7c99 to b509013 Compare June 20, 2023 08:24

github-actions bot added delta-lake Delta Lake connector iceberg Iceberg connector labels Jun 20, 2023

findepi commented Jun 20, 2023

View reviewed changes

findepi requested a review from findinpath June 20, 2023 08:26

findepi force-pushed the findepi/hive-restore-normalization branch from b509013 to 38a2eda Compare June 20, 2023 08:54

findepi mentioned this pull request Jun 20, 2023

Fix Delta writes when schema location ends with two slashes #17964

Closed

ebyhr approved these changes Jun 20, 2023

View reviewed changes

findepi force-pushed the findepi/hive-restore-normalization branch from 38a2eda to 9151dc8 Compare June 20, 2023 14:09

findepi mentioned this pull request Jun 20, 2023

Fix Delta INSERT when schema OR table location ends with two slashes #17980

Merged

findepi merged commit 431f3e0 into master Jun 20, 2023

findepi deleted the findepi/hive-restore-normalization branch June 20, 2023 21:23

github-actions bot added this to the 420 milestone Jun 20, 2023

colebow mentioned this pull request Jun 21, 2023

Add Trino 420 release notes #17997

Merged

sopel39 mentioned this pull request Aug 22, 2024

Trino Iceberg s3 path handling incompatibility when using s3a:// or s3n:// paths with // #23097

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid creating Hive tables with double slashed location #17958

Avoid creating Hive tables with double slashed location #17958

findepi commented Jun 19, 2023 •

edited

Loading

losipiuk left a comment

findinpath Jun 20, 2023 •

edited

Loading

findepi Jun 20, 2023

findepi Jun 20, 2023

findepi Jun 20, 2023

ebyhr left a comment

colebow commented Jun 21, 2023

findepi commented Jun 22, 2023

Avoid creating Hive tables with double slashed location #17958

Avoid creating Hive tables with double slashed location #17958

Conversation

findepi commented Jun 19, 2023 • edited Loading

losipiuk left a comment

Choose a reason for hiding this comment

findinpath Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

findepi Jun 20, 2023

Choose a reason for hiding this comment

findepi Jun 20, 2023

Choose a reason for hiding this comment

findepi Jun 20, 2023

Choose a reason for hiding this comment

ebyhr left a comment

Choose a reason for hiding this comment

colebow commented Jun 21, 2023

findepi commented Jun 22, 2023

findepi commented Jun 19, 2023 •

edited

Loading

findinpath Jun 20, 2023 •

edited

Loading