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

feat: support renaming table in the catalog manger #824

Merged
merged 16 commits into from
Jan 12, 2023
Merged

feat: support renaming table in the catalog manger #824

merged 16 commits into from
Jan 12, 2023

Conversation

e1ijah1
Copy link
Contributor

@e1ijah1 e1ijah1 commented Jan 4, 2023 •
edited
Loading

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

support renaming table in the catalog manger

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#723

@e1ijah1 e1ijah1 mentioned this pull request Jan 4, 2023
Closed
5 tasks
@evenyag
Copy link
Contributor

evenyag commented Jan 4, 2023

I think you could implement rename for the local catalog manager first. The remote catalog manager is a bit more complicated and let's support it in the next pr.

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jan 4, 2023

I think you could implement rename for the local catalog manager first. The remote catalog manager is a bit more complicated and let's support it in the next pr.

Ok, I will implement the remote part in the next PR.

@e1ijah1 e1ijah1 force-pushed the feat-rename-table-catalog-mgr branch from 9fa1419 to c232259 Compare January 6, 2023 05:03
@e1ijah1 e1ijah1 marked this pull request as ready for review January 6, 2023 05:04
@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jan 6, 2023

PTAL @evenyag @waynexia

@codecov
Copy link

codecov bot commented Jan 6, 2023 •
edited
Loading

Codecov Report

Merging #824 (b5487c9) into develop (0e8411c) will decrease coverage by 0.11%.
The diff coverage is 85.35%.

@@             Coverage Diff             @@
##           develop     #824      +/-   ##
===========================================
- Coverage    86.32%   86.21%   -0.12%     
===========================================
  Files          422      425       +3     
  Lines        55669    56097     +428     
===========================================
+ Hits         48058    48365     +307     
- Misses        7611     7732     +121
Flag Coverage Δ
rust 86.21% <85.35%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/catalog/src/lib.rs 88.60% <0.00%> (-1.14%) ⬇️
src/catalog/src/remote/manager.rs 68.02% <0.00%> (-3.03%) ⬇️
src/datanode/src/error.rs 67.39% <ø> (ø)
src/datanode/src/sql.rs 72.88% <0.00%> (-1.26%) ⬇️
src/frontend/src/catalog.rs 70.90% <0.00%> (-1.65%) ⬇️
src/query/src/datafusion/catalog_adapter.rs 47.97% <0.00%> (-0.74%) ⬇️
src/table/src/table.rs 25.00% <0.00%> (ø)
src/catalog/src/tables.rs 86.31% <33.33%> (+6.03%) ⬆️
src/catalog/src/local/manager.rs 72.82% <80.64%> (+4.57%) ⬆️
src/catalog/src/local/memory.rs 85.75% <93.75%> (+5.45%) ⬆️
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

evenyag
evenyag reviewed Jan 6, 2023
View reviewed changes
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

I'm able to review this PR more carefully in the next week. But It seems the changes to the key in the system_catalog table could cause unexpected problems: the database might not recover the correct table name after it restarts. I think you could address this problem first.

src/catalog/src/tables.rs Outdated Show resolved Hide resolved
src/datanode/src/sql/alter.rs Show resolved Hide resolved
@killme2008
Copy link
Contributor

@e1ijah1 Hi, is this PR ready for review? I can review it if it's ready.

@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jan 10, 2023

@e1ijah1 Hi, is this PR ready for review? I can review it if it's ready.

Yes. It's ready to review.

@e1ijah1 e1ijah1 requested a review from evenyag January 10, 2023 07:08
@evenyag evenyag requested a review from v0y4g3r January 11, 2023 02:53
@e1ijah1 e1ijah1 force-pushed the feat-rename-table-catalog-mgr branch from 80e8e91 to b0765ad Compare January 11, 2023 05:16
evenyag
evenyag reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

Most comments are related to style and the API design of rename_table()

src/catalog/src/lib.rs Outdated Show resolved Hide resolved
src/catalog/src/lib.rs Outdated Show resolved Hide resolved
src/catalog/src/schema.rs Outdated Show resolved Hide resolved
src/catalog/src/lib.rs Outdated Show resolved Hide resolved
src/catalog/src/local/memory.rs Outdated Show resolved Hide resolved
src/datanode/src/tests/test_util.rs Outdated Show resolved Hide resolved
src/datanode/src/tests/instance_test.rs Outdated Show resolved Hide resolved
src/datanode/src/tests/instance_test.rs Outdated Show resolved Hide resolved
src/mito/src/engine.rs Outdated Show resolved Hide resolved
src/mito/src/engine.rs Outdated Show resolved Hide resolved
killme2008
killme2008 reviewed Jan 11, 2023
View reviewed changes
src/catalog/src/lib.rs Outdated Show resolved Hide resolved
src/catalog/src/system.rs Show resolved Hide resolved
src/catalog/src/tables.rs Outdated Show resolved Hide resolved
src/common/telemetry/src/logging.rs Outdated Show resolved Hide resolved
@e1ijah1 e1ijah1 force-pushed the feat-rename-table-catalog-mgr branch from 9512df3 to 886c606 Compare January 11, 2023 11:45
@e1ijah1
Copy link
Contributor Author

e1ijah1 commented Jan 11, 2023

Thanks for your advice, I've fixed them all.

@killme2008
Copy link
Contributor

There is a test failure:

running 1 test
test local::memory::tests::test_catalog_rename_table ... FAILED

failures:

failures:
    local::memory::tests::test_catalog_rename_table

@e1ijah1

@e1ijah1 e1ijah1 requested review from killme2008 and evenyag and removed request for v0y4g3r and killme2008 January 12, 2023 07:00
killme2008
killme2008 approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

src/catalog/src/lib.rs Outdated Show resolved Hide resolved
evenyag
evenyag approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

LGTM

@evenyag evenyag merged commit 6775c5b into GreptimeTeam:develop Jan 12, 2023
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
@e1ijah1 @evenyag @paomian
feat: support renaming table in the catalog manger (GreptimeTeam#824)
ca9f716 
* feat: support renaming table in the catalog manger

* feat: implement rename table for local catalog manager

* chore: fmt code

* fix: update system catalog when renaming table in local catalog manager

* chore: add instance test for rename table

* chore: fix frontend test

* chore: fix comment

* chore: fix rename table test

* fix: renaming a table with an existing name

* fix: improve the system catalog's renaming process

* chore: improve the code

* chore: improve the comment

Co-authored-by: Yingwen <[email protected]>

* chore: improve the code

* chore: fix tests

* chore: fix instance_test

* chore: improve the code

Co-authored-by: Yingwen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@killme2008 killme2008 killme2008 approved these changes

@evenyag evenyag evenyag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@e1ijah1 @evenyag @killme2008