Skip to content

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

Merged
findepi merged 3 commits intotrinodb:masterfrom
findinpath:rename-changes-schema-too
Mar 23, 2022
Merged

Preserve table catalog/schema in ALTER TABLE qualified_name RENAME TO unqualified_name#11282
findepi merged 3 commits intotrinodb:masterfrom
findinpath:rename-changes-schema-too

Conversation

@findinpath
Copy link
Copy Markdown
Contributor

@findinpath findinpath commented Mar 2, 2022 •
edited
Loading

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 force-pushed the rename-changes-schema-too branch from a599643 to 933601c Compare March 2, 2022 17:33
@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
Copy Markdown
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
Copy link
Copy Markdown
Member

@findepi findepi Mar 22, 2022

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
Copy Markdown
Contributor Author

@findinpath findinpath Mar 22, 2022 •
edited
Loading

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

@findepi findepi Mar 22, 2022

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

@findepi findepi Mar 22, 2022

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
Copy Markdown
Contributor Author

@findinpath findinpath Mar 22, 2022

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

@findepi findepi Mar 22, 2022

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

@findinpath findinpath force-pushed the rename-changes-schema-too branch from 103c870 to b28412f Compare March 22, 2022 17:26
@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
Copy link
Copy Markdown
Member

@findepi findepi Mar 23, 2022

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

@findepi findepi Mar 23, 2022

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 force-pushed the rename-changes-schema-too branch from b28412f to cbbd514 Compare March 23, 2022 10:12
@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