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

Cluster table support alter #11484

Merged
merged 36 commits into from
Aug 29, 2023

Conversation

qingxinhome
Copy link
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #11437

What this PR does / why we need it:

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Aug 28, 2023
@mergify mergify bot added the kind/bug Something isn't working label Aug 28, 2023
@matrix-meow
Copy link
Contributor

@qingxinhome Thanks for your contributions!

Review of Pull Request #11437: Cluster table support alter

Summary:
This pull request aims to add support for altering cluster tables. It includes changes to the server_task.go, upgrader.go, and build_alter_table.go files. The changes involve adding a logger to the Upgrader struct in upgrader.go and removing the check for cluster tables in build_alter_table.go.

Review:

  1. In server_task.go, the change to add the logger to the Upgrader struct seems appropriate. However, it would be helpful to include a comment explaining the purpose of the logger and how it will be used.

Suggestion: Add a comment explaining the purpose of the logger and how it will be used.

  1. In upgrader.go, the addition of the logger to the Upgrader struct seems appropriate. However, it would be helpful to include a comment explaining the purpose of the logger and how it will be used.

Suggestion: Add a comment explaining the purpose of the logger and how it will be used.

  1. In upgrader.go, the addition of error logging when upgrading new table columns, new tables, and new system views is a good addition. However, the error messages could be more descriptive and provide more context about the specific error that occurred.

Suggestion: Improve the error messages to provide more context about the specific error that occurred.

  1. In build_alter_table.go, the removal of the check for cluster tables seems inappropriate. It is important to ensure that cluster tables are not altered, so this check should not be removed.

Suggestion: Revert the removal of the check for cluster tables and add a comment explaining why cluster tables cannot be altered.

Optimization:

  1. The changes made in this pull request seem to be focused on adding support for altering cluster tables. However, it would be beneficial to include some tests to ensure that the changes work as expected and do not introduce any regressions.

Suggestion: Add tests to ensure the changes work as expected and do not introduce any regressions.

Overall, this pull request makes some appropriate changes to add support for altering cluster tables. However, there are some areas that need improvement, such as adding comments and improving error messages. Additionally, the removal of the check for cluster tables should be reverted. Adding tests would also help ensure the changes work as expected.

@mergify mergify bot merged commit 29abc8d into matrixorigin:main Aug 29, 2023
3 checks passed
daviszhen pushed a commit that referenced this pull request Sep 6, 2023
sukki37 pushed a commit that referenced this pull request Sep 9, 2023
sukki37 pushed a commit that referenced this pull request Sep 10, 2023
sukki37 pushed a commit that referenced this pull request Sep 15, 2023
sukki37 pushed a commit that referenced this pull request Sep 17, 2023
sukki37 pushed a commit that referenced this pull request Sep 17, 2023
daviszhen pushed a commit to daviszhen/matrixone that referenced this pull request Sep 18, 2023
@qingxinhome qingxinhome deleted the ClusterTableSupportAlter branch September 25, 2023 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants