-
Notifications
You must be signed in to change notification settings - Fork 40
Add drop column #2983
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
Add drop column #2983
Conversation
2825fed to
a848e63
Compare
ffef3fc to
55ec710
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for dropping columns in ScalarDB by implementing the dropColumnFromTable method in the Admin interface. This enhancement allows users to remove unwanted columns without recreating entire tables.
- Adds
dropColumnFromTablemethod to theAdmininterface with validation to prevent dropping partition keys, clustering keys, or indexed columns - Implements the method across all storage adapters (JDBC, Cassandra, DynamoDB, Cosmos DB) with appropriate support or exception handling
- Includes comprehensive test coverage for various scenarios including error cases and edge conditions
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/com/scalar/db/api/Admin.java |
Adds dropColumnFromTable method definitions with validation logic |
core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java |
Implements column dropping using ALTER TABLE DROP COLUMN SQL statements |
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java |
Implements column dropping using Cassandra's SchemaBuilder |
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java |
Throws UnsupportedOperationException as DynamoDB doesn't support column dropping |
core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java |
Throws UnsupportedOperationException as Cosmos DB doesn't support column dropping |
| Various test files | Comprehensive test coverage including integration tests and unit tests for all implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java
Outdated
Show resolved
Hide resolved
...st/src/main/java/com/scalar/db/api/DistributedStorageAdminPermissionIntegrationTestBase.java
Show resolved
Hide resolved
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Left minor comments. PTAL!
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. Left minor comments/suggestions on the error messages.
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left several comments. PTAL!
core/src/main/java/com/scalar/db/common/CheckedDistributedStorageAdmin.java
Show resolved
Hide resolved
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
This PR adds support for dropping columns in ScalarDB.
Currently, ScalarDB does not support dropping columns and users must recreate tables to remove unwanted columns. This feature will provide more flexibility in managing database schemas without requiring recreating tables.
Related issues and/or PRs
ifNotExistsin add column #2960Changes made
dropColumnFromTablemethod in theAdmininterface.Checklist
Additional notes (optional)
N/A
Release notes
Added support for dropping columns in ScalarDB.