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

Add support for CREATE OR REPLACE TABLE statement #13681

Merged
merged 5 commits into from
Oct 25, 2023

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Aug 15, 2022

Support CREATE OR REPLACE syntax in engine and Iceberg connector.

Description

Fixes #13180

Is this change a fix, improvement, new feature, refactoring, or other?

new Feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Change to core query engine, SPI and connectors

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 15, 2022
@mdesmet mdesmet marked this pull request as draft August 15, 2022 20:33
@ebyhr ebyhr changed the title Feature/create or replace Add support for CREATE OR REPLACE statement Aug 15, 2022
@mdesmet mdesmet force-pushed the feature/create-or-replace branch 4 times, most recently from cb2ce4f to 6c8e05c Compare August 18, 2022 22:45
@mdesmet mdesmet marked this pull request as ready for review August 19, 2022 06:56
@findepi findepi changed the title Add support for CREATE OR REPLACE statement Add support for CREATE OR REPLACE TABLE statement Aug 23, 2022
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

On hold until consensus whether we want the feature #13180 is reached. Let's discuss there.

@wrb2
Copy link

wrb2 commented Nov 8, 2022

Hi, I think this could be unblocked, no? Or am I missing something?

@mosabua mosabua requested a review from martint March 17, 2023 17:20
@mdesmet mdesmet force-pushed the feature/create-or-replace branch 2 times, most recently from 856ab24 to 9958bdd Compare March 30, 2023 12:43
@github-actions github-actions bot added the iceberg Iceberg connector label Mar 30, 2023
@mdesmet mdesmet force-pushed the feature/create-or-replace branch from 9958bdd to 0dc5e09 Compare March 31, 2023 07:15
@findepi
Copy link
Member

findepi commented Mar 31, 2023

removed the syntax-needs-review label as it got removed from #13180.

Now that we have directional agreement, let's get this moving.
@electrum @mdesmet would it be OK if we review commits incrementally? For example, we could move first (grammar changes) commit to its own PR.

@mdesmet
Copy link
Contributor Author

mdesmet commented Apr 4, 2023

Now that we have directional agreement, let's get this moving. @electrum @mdesmet would it be OK if we review commits incrementally? For example, we could move first (grammar changes) commit to its own PR.

Please confirm this is the way to go. If not please start reviewing this PR.

@jkylling
Copy link
Contributor

We are interested in support for CREATE OR REPLACE in trino-delta-lake and have started on an implementation on top of this PR here. Thank you for the very useful PR!

@mdesmet mdesmet self-assigned this Jul 12, 2023
@mdesmet mdesmet force-pushed the feature/create-or-replace branch 2 times, most recently from 9404f7c to 0a814fc Compare July 29, 2023 09:57
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

LGTM. After applying @findinpath and @mosabua 's comment

@Praveen2112
Copy link
Member

@mosabua Can you please take a look at the docs once again.

docs/src/main/sphinx/sql/create-table-as.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/sql/create-table.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.md Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.md Outdated Show resolved Hide resolved
@mdesmet mdesmet force-pushed the feature/create-or-replace branch from 2add4d0 to f72ab79 Compare October 21, 2023 12:58
@findinpath
Copy link
Contributor

findinpath commented Oct 22, 2023

@mdesmet Build is 🔴

@mdesmet mdesmet force-pushed the feature/create-or-replace branch from f72ab79 to fe8ca65 Compare October 23, 2023 13:09
@mdesmet mdesmet force-pushed the feature/create-or-replace branch from fe8ca65 to bec23c3 Compare October 23, 2023 14:53
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Docs look good now.

@findinpath
Copy link
Contributor

Unrelated CI failure
#19446

verifyTableVersionForUpdate(table);
Table icebergTable = catalog.loadTable(session, table.getSchemaTableName());
validateNotModifyingOldSnapshot(table, icebergTable);
getTableMetadata(session, table);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this call a left-over?
Why do we need to do the call ? If there is a reasoning behind it, pls document it.

@mdesmet mdesmet force-pushed the feature/create-or-replace branch from bec23c3 to 1a0eec6 Compare October 24, 2023 11:45
@Praveen2112 Praveen2112 merged commit 2771775 into trinodb:master Oct 25, 2023
@Praveen2112
Copy link
Member

Thanks a lot @mdesmet for working on this amazing feature

@github-actions github-actions bot added this to the 431 milestone Oct 25, 2023
@findinpath
Copy link
Contributor

@jkylling road is open for Delta Lake PR

Location location = Location.of(transaction.table().location());
TrinoFileSystem fileSystem = fileSystemFactory.create(session);
try {
if (fileSystem.listFiles(location).hasNext()) {
if (!replace && fileSystem.listFiles(location).hasNext()) {
Copy link
Member

Choose a reason for hiding this comment

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

This should do the check also for replace case, when table handle was not found.

*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

It is time to remove this deprecated method. Can you please follow-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. will have a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed docs iceberg Iceberg connector needs-docs This pull request requires changes to the documentation syntax-needs-review
Development

Successfully merging this pull request may close these issues.

CREATE [OR REPLACE] TABLE support for Iceberg and Delta