Forbid scheme://authority locations (without trailing slash) by findepi · Pull Request #17923 · trinodb/trino

findepi · 2023-06-15T15:21:43Z

Locations like s3://bucket aren't consistently supported in Trino yet
and will cause compatibility issues with other software (e.g. Athena),
so forbid them. They may be allowed later.

Fixes #17921

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

findepi · 2023-06-15T19:01:16Z

@electrum @findinpath @alexjo2144 @losipiuk what is the best way to block locations like s3://bucket that are not interoperable and are partially working today?

findepi · 2023-06-15T21:13:55Z

I added commit forbidding such locations and renamed the PR accordingly.

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

findinpath · 2023-06-15T21:33:49Z

CREATE TABLE mytable WITH (external_location='s3://bucket');

It may not be straightforward for the end user to specify the table path as s3://bucket/ so that it can pass the validation.
What would be the cost to accept both s3://bucket and s3://bucket/ and treat them equivalently ?

When the metastore is HMS, the creation of the table / schema was supposed to be suffixed with /

trino> create table hive.tiny.foo1 (c integer) with (format='textfile', external_location='s3://foo1');
Query 20230615_202259_00001_mu7hf failed: java.lang.IllegalArgumentException: Can not create a Path from an empty string
	at io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastore.createTable(ThriftHiveMetastore.java:1047)
...

I'm actually still rather confused regarding the HMS behavior:

trino> create table hive.default.foo1 (c integer) with (external_location = 's3://foo1');
Query 20230615_212929_00002_7xdr9 failed: java.lang.IllegalArgumentException: Can not create a Path from an empty string
io.trino.spi.TrinoException: java.lang.IllegalArgumentException: Can not create a Path from an empty string
	at io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastore.createTable(ThriftHiveMetastore.java:1047)

trino> create table hive.default.foo1 (c integer) with (external_location = 's3://foo1/bar');
CREATE TABLE

From end user perspective, it is rather uneasy to understand the reasoning behind this functionality choice on HMS.
Both of the locations are not suffixed by /, but in case of using s3://foo1/bar as location the CREATE TABLE succeeds, but when using s3://foo1 the statement fails.

s3://foo1 and s3://foo1/ are equivalent - why not doing the necessary handling internally on Trino?

UPDATE 16.06.2023

I thought through my point about simplicity of use from end user perspective.
If the user is providing a location missing a path like s3://bucket , Trino should reject it with a meanigful message instead of accepting it.
It is better to avoid dealing with unwanted consequences of opening the door to almost correct schema/table locations.

losipiuk

LGTM

losipiuk · 2023-06-15T21:39:40Z

From end user perspective, it is rather uneasy to understand the reasoning behind this functionality choice on HMS.
s3://foo1 and s3://foo1/ are equivalent - why not doing the necessary handling internally on Trino?

Using top of the bucket as table location realistically will not be used anyway (that is my guess). It would not give us much to add behind the scenes normalization here, and it is a bit against principle of least supprise (if I specified location to X I would expect it to be set to X, not to X/).

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java

lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java

lib/trino-filesystem/src/test/java/io/trino/filesystem/local/TestLocalFileSystem.java

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

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java

hashhar

will cause compatibility issues with other software (e.g. Athena), so forbid them

IIRC our compatibility target is Hive, not Athena - which is also based on Trino. ~~And it seems Hive has no problem with such tables.~~

hashhar · 2023-06-16T06:54:28Z

Using top of the bucket as table location realistically will not be used anyway (that is my guess).

In practice I've seen a table per bucket as well.

It would not give us much to add behind the scenes normalization here, and it is a bit against principle of least supprise
(if I specified location to X I would expect it to be set to X, not to X/).

This I partially agree with - s3://bucket should be disallowed but not s3://bucket/.

stale, seems only s3://bucket stops working, not s3://bucket/

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

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java

In other places in `Location` class the location toString is not in apostrophes, so let's change this one place not to use apostrophes either.

Assert on full message, not a single word contained within.

Note: this documents current state, but does not imply the intent to support such locations.

Test Hive connector behavior when table location is top-of-bucket, but without slash at the end (`s3://bucket` and not `s3://bucket/`). As seen, the behavior is not fully coherent, so maybe it would be better to disallow such locations. FWIW, such locations aren't supported in Athena.

Locations like `s3://bucket` aren't consistently supported in Trino yet and will cause compatibility issues with other software (e.g. Athena), so forbid them. They may be allowed later.

mosabua · 2023-06-16T21:59:20Z

Question @findepi and @martint .. with that also apply to other S3 related paths and config, and specifically also the docs. For example we have

    exchange.base-directories=s3://exchange-spooling-bucket

Will that fail?

And also

    location = 's3://my-bucket/a/path',

Will that fail to? Or generically .. should we just make sure that all of these (also for others like gcs and abfs) always have the trailing slash in the docs everywhere?

findepi · 2023-06-17T16:49:25Z

For example we have
    exchange.base-directories=s3://exchange-spooling-bucket
Will that fail?

Might. I don't know.

And also
    location = 's3://my-bucket/a/path',
Will that fail to?

No. Only s3://my-bucket is impacted. s3://my-bucket/.. is not.

mosabua · 2023-06-19T17:12:08Z

For example we have
    exchange.base-directories=s3://exchange-spooling-bucket
Will that fail?
Might. I don't know.

@arhimondr @linzebing .. could you verify and maybe send a PR to update the docs?

losipiuk · 2023-06-20T09:15:10Z

Might. I don't know.

@arhimondr @linzebing .. could you verify and maybe send a PR to update the docs?

It will not - actually top of the bucket is configuration we use in unit tests.

findepi requested review from alexjo2144, ebyhr, electrum, findinpath, losipiuk and martint June 15, 2023 15:21

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

findepi commented Jun 15, 2023

View reviewed changes

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

findepi force-pushed the findepi/test-top-of-bucket-period branch from a1dde16 to dd7ddd8 Compare June 15, 2023 15:23

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

hashhar requested review from Praveen2112 and kokosing June 15, 2023 17:00

findepi mentioned this pull request Jun 15, 2023

Disallow tables with s3://bucket location (without trailing slash after bucket name) #17921

Closed

findepi marked this pull request as draft June 15, 2023 19:00

findepi force-pushed the findepi/test-top-of-bucket-period branch from dd7ddd8 to c907053 Compare June 15, 2023 21:09

findepi changed the title ~~Test Hive behavior when table location s3://bucket~~ Forbid scheme://authority locations Jun 15, 2023

findepi marked this pull request as ready for review June 15, 2023 21:10

findepi force-pushed the findepi/test-top-of-bucket-period branch from c907053 to 804b353 Compare June 15, 2023 21:13

alexjo2144 reviewed Jun 15, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestHiveS3MinioQueries.java Show resolved Hide resolved

losipiuk reviewed Jun 15, 2023

View reviewed changes

alexjo2144 reviewed Jun 15, 2023

View reviewed changes

martint reviewed Jun 15, 2023

View reviewed changes

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

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java Show resolved Hide resolved

hashhar previously requested changes Jun 16, 2023

View reviewed changes

hashhar reviewed Jun 16, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestHiveS3MinioQueries.java Show resolved Hide resolved

findinpath reviewed Jun 16, 2023

View reviewed changes

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java Outdated Show resolved Hide resolved

findinpath approved these changes Jun 16, 2023

View reviewed changes

findepi added 5 commits June 16, 2023 12:12

Test Location.parentDirectory for root paths

99f6300

Test Location.parentDirectory for folder-like locations

2f86859

Consistently not quote location in exceptions

71a09c8

In other places in `Location` class the location toString is not in apostrophes, so let's change this one place not to use apostrophes either.

Fix exception message assertions in TestLocation

de013ec

Assert on full message, not a single word contained within.

Add more location parsing tests

568c311

Note: this documents current state, but does not imply the intent to support such locations.

findepi force-pushed the findepi/test-top-of-bucket-period branch from 804b353 to 927ea05 Compare June 16, 2023 10:30

findepi mentioned this pull request Jun 16, 2023

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

Closed

1 task

findepi force-pushed the findepi/test-top-of-bucket-period branch from 927ea05 to 5508c66 Compare June 16, 2023 12:51

findepi changed the title ~~Forbid scheme://authority locations~~ Forbid scheme://authority locations (without trailing slash) Jun 16, 2023

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

c1cd116

Locations like `s3://bucket` aren't consistently supported in Trino yet and will cause compatibility issues with other software (e.g. Athena), so forbid them. They may be allowed later.

findepi force-pushed the findepi/test-top-of-bucket-period branch from 5508c66 to c1cd116 Compare June 16, 2023 13:29

findinpath assigned findepi Jun 16, 2023

findepi requested review from electrum, losipiuk and martint and removed request for electrum June 16, 2023 18:35

martint approved these changes Jun 16, 2023

View reviewed changes

findepi merged commit 4be107e into master Jun 17, 2023

findepi deleted the findepi/test-top-of-bucket-period branch June 17, 2023 16:50

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

colebow mentioned this pull request Jun 21, 2023

Add Trino 420 release notes #17997

Merged

findepi mentioned this pull request Jul 4, 2023

Hive sorted writes fail when user identity contains the @ AT sign #18132

Closed

Conversation

findepi commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

findepi commented Jun 15, 2023

Uh oh!

findepi commented Jun 15, 2023

Uh oh!

Uh oh!

findinpath commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

losipiuk commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

mosabua commented Jun 16, 2023

Uh oh!

findepi commented Jun 17, 2023

Uh oh!

mosabua commented Jun 19, 2023

Uh oh!

losipiuk commented Jun 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

findepi commented Jun 15, 2023 •

edited

Loading

findinpath commented Jun 15, 2023 •

edited

Loading

hashhar left a comment •

edited

Loading