Skip to content

Add checks for rename table and view#12402

Merged
findepi merged 1 commit intotrinodb:masterfrom
zhaoyim:i12337
Jun 13, 2022
Merged

Add checks for rename table and view#12402
findepi merged 1 commit intotrinodb:masterfrom
zhaoyim:i12337

Conversation

@zhaoyim
Copy link
Copy Markdown
Contributor

@zhaoyim zhaoyim commented May 16, 2022

Description

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

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

How would you describe this change to a non-technical end user or system administrator?
Add checks for rename table and view for target table or view or materialized view not exist

Related issues, pull requests, and links

issue: Add checks for target table.view,mv during view/table rename 12337
PR: checks for rename table and view 12402

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

Fixes #12337

@findinpath
Copy link
Copy Markdown
Contributor

Please add Fixes #12337 under:

Related issues, pull requests, and links

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use GENERIC_USER_ERROR error here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to GENERIC_USER_ERROR. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use GENERIC_USER_ERROR here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to GENERIC_USER_ERROR. Thanks!

@findinpath
Copy link
Copy Markdown
Contributor

Please squash the commits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion.

TABLE_ALREADY_EXISTS is fit in this specific case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unrelated to your PR: Can you please add a new commit for changing the error code from TABLE_ALREADY_EXISTS to GENERIC_USER_ERROR in this specific case ? (pls mind also the corresponding test case)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! also add the UT for this. Please help review again, Thanks!

Copy link
Copy Markdown
Member

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

lgtm % order of checks
Also, please squash commits.

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.

The same comment for order of checks as above in RenameTableTask

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

Thank you for your improvement.

@findinpath findinpath requested a review from findepi May 16, 2022 10:26
@findepi findepi requested review from sopel39 and removed request for sopel39 May 16, 2022 11:33
@zhaoyim
Copy link
Copy Markdown
Contributor Author

zhaoyim commented Jun 10, 2022

@findinpath @lukasz-stec Could you help merge this pr? Thanks!

@lukasz-stec
Copy link
Copy Markdown
Member

@findinpath @lukasz-stec Could you help merge this pr? Thanks!

@findepi or @sopel39 need to review it first. This may take some time.

@findepi findepi merged commit 5af3adb into trinodb:master Jun 13, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 13, 2022

thank you @zhaoyim

thanks @findinpath @lukasz-stec for your review

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jun 13, 2022
@github-actions github-actions bot added this to the 386 milestone Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

Add checks for target table.view,mv during view/table rename

4 participants