Fix secondary index behavior #3133
Conversation
Summary of ChangesHello @brfrn169, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors and strengthens the validation mechanisms within the Consensus Commit transaction protocol. By introducing a unified operation checker, it enforces more rigorous constraints on how Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces important fixes and improvements related to secondary index behavior in Consensus Commit, especially for the SERIALIZABLE isolation level.
The main changes include:
- A new
ConsensusCommitOperationCheckerclass is introduced, which consolidates and extends the validation logic forGet,Scan, andMutationoperations. This is a good refactoring that improves code organization. - New restrictions are added for the
SERIALIZABLEisolation level:GetandScanoperations using a secondary index are now disallowed.- Conditions on indexed columns in cross-partition scans (
ScanAll) are also disallowed.
- The logic for preparing scan operations in
ConsensusCommitUtilshas been updated to handle conditions on indexed columns inScanAlloperations differently, which seems to be a performance optimization. MutationConditionsValidatorhas been refactored to reduce its dependency onTransactionContext, which is a nice improvement for decoupling.
The changes are well-implemented, and the test suite has been updated comprehensively to cover the new behavior, including new integration tests that validate the new restrictions.
Overall, this is a high-quality pull request that improves the correctness and robustness of the transaction implementation. I have no further comments.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances operation checking in consensus commit transactions by introducing restrictions for SERIALIZABLE isolation level and extending the operation checker to validate read operations. The changes prevent index-based operations and conditions on indexed columns in specific scenarios when using SERIALIZABLE isolation, as these operations cannot guarantee serializability.
Key changes:
- Renames and expands
ConsensusCommitMutationOperationCheckertoConsensusCommitOperationCheckerwith support for Get/Scan validation - Adds SERIALIZABLE isolation checks to reject index-based Get/Scan operations and conditions on indexed columns in cross-partition scans
- Refactors condition conversion logic to skip indexed column conditions for ScanAll operations
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ConsensusCommitOperationChecker.java | New class replacing ConsensusCommitMutationOperationChecker with added Get/Scan validation and SERIALIZABLE isolation checks |
| ConsensusCommitMutationOperationChecker.java | Removed - replaced by ConsensusCommitOperationChecker |
| ConsensusCommit.java, TwoPhaseConsensusCommit.java | Updated to call operation checker for Get/Scan operations and use renamed checker |
| ConsensusCommitManager.java, TwoPhaseConsensusCommitManager.java | Updated to instantiate ConsensusCommitOperationChecker with include_metadata flag |
| ConsensusCommitUtils.java | Modified conjunction conversion to conditionally skip indexed columns based on scan type |
| MutationConditionsValidator.java | Simplified to accept transactionId string instead of full TransactionContext |
| CrudHandler.java | Updated to pass transactionId to MutationConditionsValidator |
| CoreError.java | Added error codes for projection/ordering metadata restrictions and SERIALIZABLE index operation restrictions |
| ConsensusCommitSpecificIntegrationTestBase.java | Updated tests to reflect SERIALIZABLE restrictions and removed obsolete index operation tests |
| Various test files | Updated to use renamed ConsensusCommitOperationChecker and adjust mock behavior |
Comments suppressed due to low confidence (1)
core/src/main/java/com/scalar/db/common/CoreError.java:439
- The error message format for
CONSENSUS_COMMIT_CONDITION_NOT_ALLOWED_TO_TARGET_TRANSACTION_METADATA_COLUMNSwas changed from accepting one parameter (Column: %s) to accepting two parameters (Table: %s; Column: %s) in the new usage, but the error enum definition was not updated. This will cause a format string mismatch. The error definition should be updated to match the new usage pattern seen in line 72 and 232 of ConsensusCommitOperationChecker.java wherebuildMessage(get.forFullTableName().get(), column)andbuildMessage(mutation.forFullTableName().get(), column)are called.
CONSENSUS_COMMIT_CONDITION_NOT_ALLOWED_TO_TARGET_TRANSACTION_METADATA_COLUMNS(
Category.USER_ERROR,
"0100",
"The condition is not allowed to target transaction metadata columns. Column: %s",
"",
""),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!convertIndexedColumns && metadata.getSecondaryIndexNames().contains(columnName)) { | ||
| conditions.add(condition); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for skipping indexed column conversion is only checking metadata.getSecondaryIndexNames().contains(columnName) but doesn't check partition key or clustering key columns (which are also indexed). According to lines 488-494, partition and clustering keys are already skipped, but the comment on line 496 and the parameter name convertIndexedColumns suggest this should handle all indexed columns. Consider renaming the parameter to convertSecondaryIndexedColumns for clarity, or verify that the current behavior matches the intended design.
| * @throws ExecutionException when retrieving the table metadata fails | ||
| * @throws IllegalArgumentException when the mutation is invalid | ||
| */ | ||
| public void check(Mutation mutation) throws ExecutionException { |
| conditionChecker.check(condition, true); | ||
| } | ||
|
|
||
| private void check(Delete delete) throws ExecutionException { |
1 participant
Description
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
Release notes