-
Notifications
You must be signed in to change notification settings - Fork 41
Allow Put after Delete on the same record in Consensus Commit transactions #3319
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
base: master
Are you sure you want to change the base?
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 enhances the flexibility of Consensus Commit transactions by modifying the behavior when a Put operation is attempted on a record that has already been deleted within the same transaction. Previously, this sequence of operations would result in an IllegalArgumentException. The new implementation intelligently handles this scenario by moving the record from the transaction's delete set to its write set, automatically nullifying unspecified non-key columns, disabling insert mode to treat it as an update, and enabling implicit pre-reads. This change streamlines "replace" operations, allowing users to delete and then re-insert or upsert records with the same key within a single, atomic transaction. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
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 introduces a significant and beneficial change to the Consensus Commit transaction model, allowing Put operations to follow Delete operations on the same record within a single transaction. This addresses a common 'replace' use case and enhances the flexibility of the transaction API. The implementation correctly handles the state transition by moving the record from the delete set to the write set, setting unspecified columns to null, disabling insert mode, and enabling implicit pre-read for proper preparation. The refactoring of createNullColumn into ScalarDbUtils improves code reusability and maintainability. All unit and integration tests have been updated to reflect the new behavior, ensuring correctness and consistency. The changes are well-implemented and align with the stated goals of the pull request.
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 modifies the Consensus Commit transaction behavior to allow Put (Insert/Upsert/Update) operations after Delete on the same record within a single transaction. Previously, this operation would throw an IllegalArgumentException.
Changes:
- Modified
Snapshot.putIntoWriteSet()to move records fromdeleteSettowriteSetwhen Put is called after Delete, with automatic null-setting for unspecified non-key columns - Extracted
createNullColumn()utility method toScalarDbUtilsfor reuse across the codebase - Removed unused error code
CONSENSUS_COMMIT_WRITING_ALREADY_DELETED_DATA_NOT_ALLOWEDfromCoreError.java
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java | Modified putIntoWriteSet() to handle Put after Delete by moving records from deleteSet to writeSet with null columns for unspecified fields |
| core/src/main/java/com/scalar/db/util/ScalarDbUtils.java | Added createNullColumn() utility method to create null columns for all supported data types |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/MergedResult.java | Refactored to use ScalarDbUtils.createNullColumn() instead of private getNullColumn() method |
| core/src/main/java/com/scalar/db/common/CoreError.java | Removed unused error code CONSENSUS_COMMIT_WRITING_ALREADY_DELETED_DATA_NOT_ALLOWED |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/SnapshotTest.java | Updated tests to verify new behavior with comprehensive test cases for Put after Delete scenarios |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerTest.java | Updated method signatures to propagate CrudException |
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CommitHandlerWithGroupCommitTest.java | Updated method signatures to propagate CrudException |
| integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitIntegrationTestBase.java | Updated integration tests to verify Put after Delete produces correct results with null columns |
| integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java | Updated integration tests to verify new behavior instead of expecting exceptions |
| integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitSpecificIntegrationTestBase.java | Updated integration tests to verify new behavior instead of expecting exceptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1c612b7 to
79fd3a2
Compare
Description
This PR changes the behavior of
Snapshot.putIntoWriteSet()when a Put (Insert/Upsert/Update) operation is performed on a record that was previously deleted within the same transaction.Previous behavior
IllegalArgumentExceptionwhen Put was called after Delete on the same recordNew behavior
deleteSettowriteSetUse case
This change enables scenarios where a user wants to delete a record and then insert/upsert a new record with the same key within a single transaction, which is a common pattern in "replace" operations.
Related issues and/or PRs
N/A
Changes made
Snapshot.java: ModifiedputIntoWriteSet()to handle Put after DeleteCoreError.java: Removed unused error codeCONSENSUS_COMMIT_WRITING_ALREADY_DELETED_DATA_NOT_ALLOWEDSnapshotTest.java: Added/updated unit tests for the new behaviorCommitHandlerTest.java,CommitHandlerWithGroupCommitTest.java: Updated method signatures to propagateCrudExceptionChecklist
Additional notes (optional)
N/A
Release notes
Changed the behavior of Consensus Commit transactions to allow Insert/Upsert/Update operations after Delete on the same record within the same transaction. Previously, this operation threw an
IllegalArgumentException, but now it properly handles the case by treating it as a new record insertion with null values for unspecified columns.