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.

Sign up for GitHub

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

Jump to bottom

Preserve table catalog/schema in ALTER TABLE qualified_name RENAME TO unqualified_name #11282

Merged
merged 3 commits into from Mar 23, 2022
Merged

Preserve table catalog/schema in ALTER TABLE qualified_name RENAME TO unqualified_name #11282

merged 3 commits into from Mar 23, 2022

Conversation

findinpath
Copy link
Contributor

@findinpath findinpath commented Mar 2, 2022 •
edited

Description

When performing ALTER TABLE xxx RENAME TO yyy deduce the qualified object name of yyy from the session and use the source as a fallback

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

This is a bugfix.

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

Fix.

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

See #11281

Related issues, pull requests, and links

Fixes #11281

Documentation

(x) 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

(x) 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 Mar 2, 2022
@findinpath findinpath changed the title WIP: Deduce the target qualified object name from session and source WIP: Deduce the target qualified object name from the source table name Mar 2, 2022
@findinpath findinpath changed the title WIP: Deduce the target qualified object name from the source table name Deduce the target qualified object name from the source table name Mar 2, 2022
@findinpath findinpath mentioned this pull request Mar 2, 2022
Merged
@findepi findepi changed the title Deduce the target qualified object name from the source table name Preserve table catalog/schema in ALTER TABLE qualified_name RENAME TO unqualified_name Mar 4, 2022
@findepi
Copy link
Member

findepi commented Mar 4, 2022

The CI is red.

@findinpath findinpath force-pushed the rename-changes-schema-too branch 3 times, most recently from ff4914f to 103c870 Compare March 10, 2022 11:23
@findinpath findinpath mentioned this pull request Mar 22, 2022
Closed
findepi
findepi reviewed Mar 22, 2022
View reviewed changes
...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeMinioConnectorTest.java Outdated
@Override
public void testRenameTableUnqualifiedName()
{
// override needed to provide valid location for schema (it needs to be s3:// based). By default schema is assigned hdfs:// location
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a method in BCT to fill in required table properties (default: none), so that we don't need to override such nice and verbose test method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for seeing this cleaner way to customize the tests for delta lake.
By using a createSchema(String schemaName) method, we can spare the duplication of the test methods.

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeMinioConnectorTest.java Outdated
{
// override needed to provide valid location for schema (it needs to be s3:// based). By default schema is assigned hdfs:// location
// TODO: better fix would be to configure hadoop container we use in test to use `s3://` filesystem by default
skipTestUnless(hasBehavior(SUPPORTS_CREATE_SCHEMA) && hasBehavior(SUPPORTS_CREATE_TABLE));
Copy link
Member

Choose a reason for hiding this comment

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

add empty line after // TODO: better fix would, since the code line and comment are unrelated

Copy link
Member

Choose a reason for hiding this comment

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

BTW skipTestUnless is not needed in a specific test class (here), because it knows what it does and doesn't support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not applying anymore because the duplicated test method has been removed from the class.

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated
@@ -1674,6 +1674,36 @@ public void testRenameTableAcrossSchema()
assertFalse(getQueryRunner().tableExists(getSession(), renamedTable));
}

@Test
public void testRenameTableUnqualifiedName()
Copy link
Member

Choose a reason for hiding this comment

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

in prep commit move testRenameTableAcrossSchema directly below testRenameTable
and then add this new test after the two

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved
@findinpath findinpath requested a review from findepi March 22, 2022 17:27
findepi
findepi reviewed Mar 23, 2022
View reviewed changes
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated

String renamedTable = "test_rename_unqualified_name_new_" + randomTableSuffix();
assertUpdate("ALTER TABLE " + sourceSchemaName + "." + tableName + " RENAME TO " + renamedTable);
assertFalse(getQueryRunner().tableExists(getSession(), tableName));
Copy link
Member

Choose a reason for hiding this comment

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

actually, assertFalse(getQueryRunner().tableExists(getSession(), tableName)); would succeed even before rename. You don't pass sourceSchemaName here (and you cannot).

For simplicity, let's skip the assertion.

Copy link
Member

Choose a reason for hiding this comment

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

(applied)

@findinpath @findepi
Extrapolate the target qualified object name from the source table
cbbd514 
When dealing with unqualified object names for the target
table, use the source table name for deducing the target
qualified object name.
@findepi findepi merged commit 37b6f36 into trinodb:master Mar 23, 2022
@findepi findepi mentioned this pull request Mar 23, 2022
Closed
@github-actions github-actions bot added this to the 375 milestone Mar 23, 2022
@mosabua mosabua mentioned this pull request Mar 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

@hashhar hashhar Awaiting requested review from hashhar

@martint martint Awaiting requested review from martint

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed
Milestone
375
Development

Successfully merging this pull request may close these issues.

ALTER TABLE RENAME implicitly changes table schemata
2 participants
@findinpath @findepi