-
Notifications
You must be signed in to change notification settings - Fork 40
Add alter column type tests for imported tables #3058
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
Conversation
6d7111d
to
f5e715e
Compare
/gemini review |
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.
Code Review
This PR adds integration tests for altering column types on imported tables. The changes look good overall, with new tests covering type alteration to TEXT and widening conversions for various database backends. I've identified some opportunities for improvement:
- There is duplicated code in the test setup logic that could be refactored into a helper method for better maintainability.
- One of the new SQL Server tests has an inconsistency between its 'act' and 'assert' phases, which could lead to an incomplete verification.
My detailed comments are below.
...rc/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTableIntegrationTest.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java
Show resolved
Hide resolved
...rc/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTableIntegrationTest.java
Show resolved
Hide resolved
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 integration tests for the alter column type feature when applied to imported database tables. The tests verify that column type alterations work correctly for tables that were imported from existing databases, covering both conversion to TEXT and widening conversions (INT to BIGINT, FLOAT to DOUBLE).
Key changes:
- Added two new test methods in the base test class to verify alter column type operations on imported tables
- Implemented helper methods to identify columns compatible with widening conversions for each supported database engine
- Added database-specific test implementations to handle engine-specific limitations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
DistributedStorageAdminImportTableIntegrationTestBase.java | Added base test methods for alter column type operations and abstract helper methods for retrieving compatible column names |
JdbcEnv.java | Added isSqlServer() helper method to detect SQL Server databases |
JdbcAdminImportTestUtils.java | Implemented column compatibility helper methods for each database engine (MySQL, PostgreSQL, Oracle, SQL Server, DB2) |
JdbcAdminImportTableIntegrationTest.java | Added concrete test implementations with database-specific handling for engines with limited type conversion support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...rc/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTableIntegrationTest.java
Show resolved
Hide resolved
...rc/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTableIntegrationTest.java
Show resolved
Hide resolved
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!
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 a question. Other than that, LGTM! Thank you!
...rc/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTableIntegrationTest.java
Show resolved
Hide resolved
core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcAdminImportTestUtils.java
Show resolved
Hide resolved
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!
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 alter column type tests for imported tables.
Imported tables can have columns with data types different from what ScalarDB regularly expects in the underlying storage. For example, in MySQL, not only
INT
but alsoINTEGER
,MEDIUMINT
, andSMALLINT
are mapped to ScalarDB'sINT
. Therefore, we need to verify that the alter column type feature works for all possible data types that imported tables can have.Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A