Skip to content
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

Allow for CTAS table creation to external tables when write and creates are enabled for unmanaged tables #16147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashishtadose
Copy link
Contributor

…-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.

Referred Trino #2669 and follow up fix #2669 & #3755

Co-authored-by: alexjo2144 jo.alex2144@gmail.com

Test plan - (Please fill in how you tested your changes)

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* ...
* ...

Hive Changes
* ...
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

…-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.

Referred Trino prestodb#2669 and follow up fix prestodb#2669 & prestodb#3755

Co-authored-by: alexjo2144 <jo.alex2144@gmail.com>
@aweisberg
Copy link
Contributor

The first line of the commit message is too long (should not be truncated in GH UI) , and there is no release note. @yingsu00 I saw you added yourself as a reviewer to #15990, but maybe we should go with this one because it includes additional tests from Trino?

@aweisberg
Copy link
Contributor

There is another commit missing trinodb/trino@883e38d
When backporting from Trino it's usually a good idea to look at Trino master in addition to the original PR so you can see everything that has been changed/fixed since it was originally introduced.

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

LGTM modulus the other comments and the fix we might want.

&& !isS3FileSystem(context, hdfsEnvironment, path);
&& !isS3FileSystem(context, hdfsEnvironment, path)
// Skip using temporary directory if destination is external. Target may be on a different file system.
&& !externalLocation.isPresent();
Copy link
Contributor

Choose a reason for hiding this comment

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

This was tweaked to be externalLocation.isEmpty() in Trino.

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

LGTM modulus the other comments and the fix we might want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow mixed case in from base functions
2 participants