Create external table location for Hive

[vlad-lyutenko]

Description

Create directory structure if it's not exists.
external location will be created if writesToNonManagedTablesEnabled flag is set

The same behaviour is on pure Hive as mentioned in #17920 (comment)

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

With current change - external location will be created for Hive tables if flag hive.non-managed-table-writes-enabled is set,
otherwise exception will raised as it was before.

# Hive
* Create an empty directory if the external location doesn't exist

[krvikash]

LGTM. nitpick comments only.

[vlad-lyutenko]

LGTM. nitpick comments only.

Big thanks, all comments addressed

[ebyhr]

I would recommend reading #1277. I don't think we want to allow creating the directory with external_location table property.

[krvikash]

It seems the commit message is separated into two lines. Could you please make it in one line?

[vlad-lyutenko]

I would recommend reading #1277. I don't think we want to allow creating the directory with external_location table property.

@ebyhr thx, for discussion

Maybe we can introduce this change with some config toggle like:
@Config("hive.allow-create-external-table-path")
?

[tooptoop4]

need a way to disable this for following reasons:

1. creating a table that points to an existing s3 location that contains sensitive data would circumvent table controls. ie create table myschema.sneakytable WITH (external_location = 's3://sensitive_bucket/data_only_other_team_should_see/')
2. creating a table that changes/deletes/overwrites data on some existing s3 location

[tooptoop4]

need config to prevent

[hashhar]

creating a table that points to an existing s3 location that contains sensitive data would circumvent table controls. ie create table myschema.sneakytable WITH (external_location = 's3://sensitive_bucket/data_only_other_team_should_see/')

For this there is hive.non-managed-table-creates-enabled and hive.non-managed-table-writes-enabled.

This change does allow a user to create a table pointing to a non-existent location though because the directory would get auto-created.

Also I agree with Yuya on that we shouldn't add this. Hive doesn't allow this for example and also because "Create external table target location, if it's not exists." is what a managed table is - not an external table. External tables are supposed to point to a table which already exists but isn't registered. And for allowing arbitrary location for managed tables see concerns in the issue and the two PRs linked from that issue that Yuya has shared.

[vlad-lyutenko]

For me this change is more about, synchronise behaviour with s3:

 private void checkExternalPath(HdfsContext context, Path path)
    {
        try {
            if (!isS3FileSystem(context, hdfsEnvironment, path)) {

As far as I understand on s3 we don't have abstraction as directory, so if for example someone wants to create external table, he could just provide any non existing path and we even don't check it (exists or not exists), because this path will be automatically "created" by s3 (path will be just part of the key).

So I'd like to give the same option for HDFS, if we want external table just give trino a path and we will create it.

[vlad-lyutenko]

External tables are supposed to point to a table which already exists but isn't registered

I mean, that I could not find code which actually check that table exists on provided external_path, it's just check that path exists (and only for non s3 file systems).

[vlad-lyutenko]

I would recommend reading #1277.

Yes and this is good option to allow create managed table with location provided by the user.
But this will require to disallow create external table on path, where no table exists (not just path not exists).

So as soon, as it will be implemented and merged, it will eliminate current change, but mean time current change just synchronise behaviours between s3 and hdfs.

[Praveen2112]

Hive doesn't allow this for example and also because "Create external table target location, if it's not exists."

Last time when I checked with Hive - if create an external table pointing to non-existing directory, hive would create an empty directory

#1277 is about setting the location of the manged table while this PR is about external table - I'm not sure if it would help us here.

[Praveen2112]

Can we have additional PT coverage where we check the permission of the directory for external tables ? We do have a similar test for hdfs-impersonation

[vlad-lyutenko]

. We should instead separate these into two different properties.

so we should introduce smth like external_location_create_if_not_exists (or some more appropriate name)?

[vlad-lyutenko]

All new code needs to use TrinoFileSystem.

didn't find how to create directory using TrinoFileSystem,

or we should add method like createDirecotry in TrinoFileSystem ?

[Praveen2112]

We should NOT create a non-managed location, as by definition it’s not managed by us.

@electrum Please correct me if I am wrong, this definition is derived from hive right, so if hive supports creating an empty directory if it doesn't exist shouldn't (for an external table) we support the same. Today we do support inserting data into a new partition based on writesToNonManagedTablesEnabled flag can't we exhibit a similar behavior for create table as well.

This is why we don’t allow writing to external locations by default.

Even in this PR we do hide them behind a flag which is disabled by default.

[Praveen2112]

Discussed offline with @electrum , now external location will be created if writesToNonManagedTablesEnabled flag is set.

[Praveen2112]

/test-with-secrets sha=39aa31743ed6cd83b0fcd785e22c43f450783ceb

[github-actions]

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5961189431

[Praveen2112]

/test-with-secrets sha=883ae60c59bafb1f4d7dfb6dafb0ad8db2e1c602

[github-actions]

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5961868678

[Praveen2112]

There were a few flaky failures, re-ran them, now the pipeline is green

[Praveen2112]

Thanks for working on this.

[findepi]

why this exclusion?

[vlad-lyutenko]

to be honest, don't remember any specific reason, need to run and check this test

[findepi]

understood. please recover this info & capture it as a code comment.

[vlad-lyutenko]

understood. please recover this info & capture it as a code comment.

got it I didn't want enable hive.non-managed-table-writes-enabled for whole EnvMultinode environment to break other tests,
which is required for this test

[vlad-lyutenko]

#20936