-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Allow for CTAS table creation to external tables when write are enabled #2669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTables.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTables.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTables.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTables.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTables.java
Outdated
Show resolved
Hide resolved
|
Good job, a bunch of editorial comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@alexjo2144 could you please squash the commits now, without rebasing on current master?
on the command line, git rebase --interactive $(git merge-base HEAD master) should do the trick.
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveCreateExternalTable.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, I think here we should check both: writesToNonManagedTablesEnabled and createsOfNonManagedTablesEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but any idea why that case isn't being caught by https://github.com/prestosql/presto/blob/master/presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java#L789
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIR that place is only for create without data (one-off create)
and this code is only for create with data (prepare-write-commit)
66d90fb to
f3ef1a3
Compare
|
Rebased, added the create external tables enabled check, and added a few more tests to cover the case for writes enabled but creates disabled. |
…tes are enabled. When hive.non-managed-table-writes-enabled and hive.non-managed-table-creates-enabled are both set to true, external tables can be created and inserted into. This change allows for that to be done in one step using the `CREATE TABLE AS` syntax.
f3ef1a3 to
855a9d2
Compare
|
Merged, thanks! |
Duplicates #2556 with tests added.
When
hive.non-managed-table-writes-enabledandhive.non-managed-table-creates-enabledare both set to true, external tables can be created and inserted into. This change
allows for that to be done in one step using the
CREATE TABLE ASsyntax.