Skip to content

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

Merged
phd3 merged 1 commit intotrinodb:masterfrom
weijiii:create_table_without_data
Nov 14, 2022
Merged

Do not create temporary staging path for CREATE TABLE statement#14830
phd3 merged 1 commit intotrinodb:masterfrom
weijiii:create_table_without_data

Conversation

@weijiii
Copy link
Copy Markdown
Member

@weijiii weijiii commented Oct 30, 2022

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
Copy link
Copy Markdown
Member Author

weijiii commented Oct 30, 2022

cc: @phd3

@weijiii weijiii force-pushed the create_table_without_data branch 2 times, most recently from 50a62c3 to 149d23e Compare October 31, 2022 01:40
@weijiii weijiii changed the title Create table without data Do not create temporary staging path for CREATE TABLE statement Oct 31, 2022
@phd3 phd3 self-requested a review October 31, 2022 20:55
@weijiii weijiii force-pushed the create_table_without_data branch 2 times, most recently from 8c9edbb to 4c09730 Compare November 2, 2022 08:57
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Comment on lines 3008 to 3009
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inline

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: split args once above is inlined

Copy link
Copy Markdown
Member Author

@weijiii weijiii Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@weijiii weijiii Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@phd3 phd3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense; I'll make the change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@weijiii weijiii force-pushed the create_table_without_data branch from dc98ad6 to 3e85252 Compare November 14, 2022 04:29
@weijiii weijiii force-pushed the create_table_without_data branch from 3e85252 to 792310e Compare November 14, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants