-
Notifications
You must be signed in to change notification settings - Fork 40
Backport to branch(3) : Add validation for secondary indexes in table metadata with Object Storage #3138
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
|
/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 pull request correctly adds validation to disallow secondary indexes for Object Storage, as it doesn't support them. The core logic is sound, and the integration tests are updated accordingly. The base test classes are also refactored to better support this change. My review includes a couple of suggestions to reduce code duplication in the test files for improved maintainability.
...gration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageAdminIntegrationTest.java
Show resolved
Hide resolved
...orage/objectstorage/SingleCrudOperationTransactionAdminIntegrationTestWithObjectStorage.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 pull request removes secondary index support from Object Storage implementation by disallowing tables with secondary indexes to be created or repaired. The change adds validation in ObjectStorageAdmin to reject table metadata containing secondary indexes, since Object Storage doesn't support index functionality.
- Adds
checkTableMetadata()validation method to reject tables with secondary indexes - Updates test base classes to make
getTableMetadata()overridable - Modifies Object Storage-specific tests to override
getTableMetadata()and remove secondary indexes
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ObjectStorageAdmin.java | Adds validation method to reject table metadata with secondary indexes in createTable() and repairTable() |
| DistributedTransactionAdminIntegrationTestBase.java | Makes getTableMetadata() overridable and updates tests to use the overridable method |
| DistributedStorageAdminIntegrationTestBase.java | Updates tests to use overridable getTableMetadata() for flexibility |
| DistributedTransactionAdminRepairTableIntegrationTestBase.java | Removes secondary indexes from repair table test metadata |
| DistributedStorageAdminRepairTableIntegrationTestBase.java | Changes column name visibility and removes secondary indexes from repair table test metadata |
| SingleCrudOperationTransactionAdminIntegrationTestWithObjectStorage.java | Overrides getTableMetadata() to remove secondary indexes for Object Storage |
| ObjectStorageAdminIntegrationTest.java | Overrides getTableMetadata() to provide metadata without secondary indexes |
| ObjectStorageAdminCaseSensitivityIntegrationTest.java | Overrides getTableMetadata() to provide metadata without secondary indexes |
| ConsensusCommitAdminIntegrationTestWithObjectStorage.java | Overrides getTableMetadata() to remove secondary indexes for Object Storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is an automated request for a manual backport of the following:
Thank you!