Add rename table #3021
Add rename table #3021
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for renaming tables in ScalarDB. The changes include adding a renameTable method to the Admin interface and implementing it for various storage backends. For storages that do not support table renaming (Cassandra, CosmosDB, DynamoDB), an UnsupportedOperationException is thrown. For JDBC-based storages, the functionality is implemented using native SQL commands. The PR also includes comprehensive unit and integration tests for the new feature. The changes are well-implemented, but I have one suggestion to improve consistency in error handling.
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for renaming tables in ScalarDB by implementing the renameTable method across all admin interfaces and storage implementations. It includes proper validation for existing source tables and prevents conflicts with existing destination table names.
- Added
renameTablemethod to the Admin interface with proper error handling - Implemented the functionality across all storage adapters (JDBC, Cassandra, DynamoDB, Cosmos DB)
- Added comprehensive test coverage including unit tests and integration tests
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/api/Admin.java | Added renameTable method definition to the Admin interface |
| core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java | Implemented renameTable with table metadata handling and SQL execution |
| core/src/main/java/com/scalar/db/storage/jdbc/RdbEngine*.java | Added database-specific SQL generation for table renaming across all RDB engines |
| core/src/main/java/com/scalar/db/storage/*/Admin.java | Added UnsupportedOperationException for non-JDBC storages (Cassandra, DynamoDB, Cosmos) |
| core/src/main/java/com/scalar/db/common/CommonDistributedStorageAdmin.java | Added validation logic for table existence and conflicts |
| integration-test/src/main/java/**/Test.java | Added comprehensive integration tests for all scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...integration-test/java/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
KodaiD
requested review from
a team,
Torch3333,
brfrn169,
feeblefakie and
komamitsu
and removed request for
a team
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for renaming tables. The changes look good overall, with a new renameTable method in the Admin interface and implementations for various storage backends. I've found a couple of areas for improvement. One is a missing check for the existence of the new table name in ConsensusCommitAdmin, which could lead to unexpected behavior. The other is an opportunity to use a centralized error message for consistency in JdbcAdmin. Please see my detailed comments.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...-test/java/com/scalar/db/storage/cassandra/CassandraAdminCaseSensitivityIntegrationTest.java
Show resolved
Hide resolved
...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Outdated
Show resolved
Hide resolved
...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java
Show resolved
Hide resolved
...st/src/main/java/com/scalar/db/api/DistributedStorageAdminPermissionIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
KodaiD
force-pushed
the
add-rename-table
branch
from
d04eb35 to
1f94250
Compare
There was a problem hiding this comment.
LGTM! Thank you!
Although it’s not directly related to this PR, we might need to revisit the repairTable logic. The repairTable method is used when DDL operations fail, and since we’re extending alter table operations, we may encounter more failure cases that need to be handled.
Could you please take a look at it when you have time? 🙇
|
@KodaiD Could you please fix the conflicts? |
6 participants
Description
This PR adds support for renaming tables in ScalarDB.
Currently, ScalarDB does not support renaming tables and users must recreate tables to rename tables. This feature will provide more flexibility in managing database schemas without requiring recreating tables.
Related issues and/or PRs
ifNotExistsin add column #2960Changes made
renameTablemethod in the Admin interface.Checklist
Additional notes (optional)
N/A
Release notes
Added support for renaming tables in ScalarDB.