Do not create temporary staging path for CREATE TABLE statement #14830

weijiii · 2022-10-30T23:52:59Z

Description

I am continuing the work in #5802 .
The issue is that both CREATE TABLE and CREATE TABLE AS ... use forNewTable to generate a LocationHandle. In subsequent operations the directory created for CREATE TABLE is never renamed as the target directory because there was no data written to it.
The solution is to not create any temporary staging directory for CREATE TABLE operations. As suggested by the previous discussion I decided to create a new API forNewTableAsSelect in LocationService, and modify the original forNewTable to not create any temporary staging directory as it should.

Non-technical explanation

Currently during CREATE TABLE operation, Trino creates a unnecessary directory that is not used and cleaned up afterwards. This patch will fix this issue.

(x) 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.
( ) Release notes are required, with the following suggested text:

weijiii · 2022-10-30T23:53:19Z

cc: @phd3

phd3 · 2022-11-03T14:59:13Z

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

@@ -27,6 +27,8 @@
 {
    LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation);


l'd simplify this as below - as we don't care about external location anyway.

Path forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName);

phd3 · 2022-11-03T15:03:51Z

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

+                            .setBucketProperty(Optional.empty())
+                            .setSerdeParameters(ImmutableMap.of());


nit: if nothing specific being set - better to remove as this is default ?

phd3 · 2022-11-03T15:04:10Z

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

+                                    PRESTO_VERSION_NAME, TEST_SERVER_VERSION,
+                                    PRESTO_QUERY_ID_NAME, session.getQueryId()))
+                            .setDataColumns(columns)
+                            .setPartitionColumns(ImmutableList.of());


phd3 · 2022-11-03T15:05:14Z

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

+                            .setStorageFormat(StorageFormat.create(storageFormat.getSerde(), storageFormat.getInputFormat(), storageFormat.getOutputFormat()))
+                            .setBucketProperty(Optional.empty())
+                            .setSerdeParameters(ImmutableMap.of());
+                    PrincipalPrivileges principalPrivileges = testingPrincipalPrivilege(tableOwner, session.getUser());


nit: inline

phd3 · 2022-11-03T15:05:26Z

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

+                            .setBucketProperty(Optional.empty())
+                            .setSerdeParameters(ImmutableMap.of());
+                    PrincipalPrivileges principalPrivileges = testingPrincipalPrivilege(tableOwner, session.getUser());
+                    transaction.getMetastore().createTable(session, tableBuilder.build(), principalPrivileges, Optional.empty(), Optional.empty(), true, EMPTY_TABLE_STATISTICS, false);


nit: split args once above is inlined

@phd3 I'll address these comments in next revision.

@phd3 I have made changes according to your comments above. Please take a look when you have time. Thanks!

IIUC this comment is suggesting to change split the arguments of the method call on line 3011 to separate lines with principalPrivileges inlined.

phd3

lgtm, just one comment

phd3 · 2022-11-08T14:39:59Z

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

@@ -341,6 +341,8 @@

    protected static final String TEST_SERVER_VERSION = "test_version";

+    protected static final String FIXED_TEMPORARY_STAGING_PREFIX = "fixed-hive-staging-prefix";


I'd actually use a test-local prefix (with a random suffix) - as any reuse of this static variable in future tests can potentially break the assertion

Makes sense; I'll make the change.

@phd3 I've addressed this change in the new commit. Please take a look when you have time. Thanks!

cla-bot bot added the cla-signed label Oct 30, 2022

github-actions bot added the tests:hive label Oct 30, 2022

weijiii requested review from electrum and findepi October 30, 2022 23:53

weijiii force-pushed the create_table_without_data branch 2 times, most recently from 50a62c3 to 149d23e Compare October 31, 2022 01:40

weijiii changed the title ~~Create table without data~~ Do not create temporary staging path for CREATE TABLE statement Oct 31, 2022

phd3 self-requested a review October 31, 2022 20:55

ebyhr mentioned this pull request Nov 1, 2022

Do not create temp staging directory for CREATE TABLE without data #5802

Closed

weijiii force-pushed the create_table_without_data branch 2 times, most recently from 8c9edbb to 4c09730 Compare November 2, 2022 08:57

phd3 approved these changes Nov 3, 2022

View reviewed changes

weijiii force-pushed the create_table_without_data branch 3 times, most recently from 967e690 to dc98ad6 Compare November 7, 2022 20:52

phd3 approved these changes Nov 8, 2022

View reviewed changes

weijiii force-pushed the create_table_without_data branch from dc98ad6 to 3e85252 Compare November 14, 2022 04:29

Do not create temporary staging path for CREATE TABLE statement

792310e

weijiii force-pushed the create_table_without_data branch from 3e85252 to 792310e Compare November 14, 2022 06:56

phd3 approved these changes Nov 14, 2022

View reviewed changes

phd3 merged commit ca67fa4 into trinodb:master Nov 14, 2022

github-actions bot added this to the 403 milestone Nov 14, 2022

colebow mentioned this pull request Nov 15, 2022

Add Trino 403 release notes #14977

Merged

raunaqmorarka mentioned this pull request May 8, 2023

Remove session property overrides for Hive staging location #17390

Merged

byungnam mentioned this pull request Jul 11, 2023

CREATE TABLE leaves /tmp/presto-{USER}/xxx #4868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not create temporary staging path for CREATE TABLE statement #14830

Do not create temporary staging path for CREATE TABLE statement #14830

weijiii commented Oct 30, 2022

weijiii commented Oct 30, 2022

phd3 Nov 3, 2022

phd3 Nov 3, 2022

phd3 Nov 3, 2022

phd3 Nov 3, 2022

phd3 Nov 3, 2022

weijiii Nov 4, 2022 •

edited

Loading

weijiii Nov 8, 2022

weijiii Nov 8, 2022 •

edited

Loading

phd3 left a comment

phd3 Nov 8, 2022

weijiii Nov 8, 2022

weijiii Nov 14, 2022

		@@ -27,6 +27,8 @@
		{
		LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation);

		.setBucketProperty(Optional.empty())
		.setSerdeParameters(ImmutableMap.of());

		@@ -341,6 +341,8 @@

		protected static final String TEST_SERVER_VERSION = "test_version";

		protected static final String FIXED_TEMPORARY_STAGING_PREFIX = "fixed-hive-staging-prefix";

Do not create temporary staging path for CREATE TABLE statement #14830

Do not create temporary staging path for CREATE TABLE statement #14830

Conversation

weijiii commented Oct 30, 2022

Description

Non-technical explanation

weijiii commented Oct 30, 2022

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

weijiii Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weijiii Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

phd3 left a 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

weijiii Nov 4, 2022 •

edited

Loading

weijiii Nov 8, 2022 •

edited

Loading