Add validation for secondary indexes in table metadata with Object Storage #3127
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 effectively adds validation to ObjectStorageAdmin to prevent the use of unsupported secondary indexes. The corresponding updates to the integration tests are well-executed, particularly the refactoring of base test classes to accommodate storage-specific metadata, which improves test extensibility. I have one piece of feedback regarding an error message in ObjectStorageAdmin.java that seems to have been copied from another storage implementation and could be misleading.
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java
Outdated
Show resolved
Hide resolved
KodaiD
requested review from
a team,
Torch3333,
brfrn169,
Copilot,
feeblefakie and
komamitsu
and removed request for
a team
There was a problem hiding this comment.
Pull Request Overview
This PR adds metadata validation to ObjectStorage to prevent unsupported features like secondary indexes and BLOB clustering keys. The changes introduce a checkMetadata() method in ObjectStorageAdmin that validates table metadata during table creation and repair operations. Integration test base classes are modified to allow subclasses to override table metadata, enabling ObjectStorage-specific tests to provide custom metadata without secondary indexes.
- Adds validation in
ObjectStorageAdminto reject secondary indexes and BLOB-type clustering keys - Introduces a new error constant
OBJECT_STORAGE_CLUSTERING_KEY_BLOB_TYPE_NOT_SUPPORTED - Refactors integration test base classes to support customizable table metadata via
getTableMetadata()method
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ObjectStorageAdmin.java | Implements checkMetadata() validation method and integrates it into createTable() and repairTable() |
| CoreError.java | Adds new error constant for BLOB clustering key validation |
| DistributedTransactionAdminRepairIntegrationTestBase.java | Changes visibility of COL_NAME12-15 constants from private to protected |
| DistributedTransactionAdminIntegrationTestBase.java | Adds getTableMetadata() method and updates tests to use it for flexible metadata |
| DistributedStorageAdminRepairIntegrationTestBase.java | Changes visibility of all COL_NAME constants from private to protected |
| DistributedStorageAdminIntegrationTestBase.java | Updates tests to use dynamic metadata from getTableMetadata() |
| ObjectStorage integration test files | Override getTableMetadata() to provide metadata without secondary indexes for ObjectStorage tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively adds validation to prevent the use of unsupported secondary indexes with Object Storage by checking the table metadata during table creation and repair. The changes in ObjectStorageAdmin.java are clear and correctly placed. The updates to integration tests to remove secondary indexes are necessary and well-executed. Additionally, the refactoring in the base test classes to make secondary index assertions more dynamic is a welcome improvement to test maintainability. I've provided a few suggestions to further improve the test code by simplifying assertions and reducing duplication.
...gration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageAdminIntegrationTest.java
Show resolved
Hide resolved
...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
5 participants
Description
This PR adds validation for secondary indexes in table metadata with Object Storage. Some integration tests are also updated to prepare table metadata without secondary indexes.
Related issues and/or PRs
Changes made
ObjectStorageAdminto include validation for secondary indexes in the specified table metadata.Checklist
Additional notes (optional)
N/A
Release notes
N/A