-
Notifications
You must be signed in to change notification settings - Fork 0
Add Merge Operators support #45
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
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 adds comprehensive support for RocksDB merge operators, enabling efficient atomic read-modify-write operations. The implementation introduces a new inheritance hierarchy with RocksDbStoreBase as the common base class, from which both the existing RocksDbStore and new MergeableRocksDbStore derive. This allows stores to perform atomic operations like incrementing counters or modifying lists without requiring separate read-before-write operations.
Key changes include:
- Introduction of
MergeableRocksDbStore<TKey, TValue, TOperand>base class for stores that support merge operations - Implementation of
ListMergeOperator<T>with support for atomic add/remove operations on lists - Refactoring of
RocksDbStoreinto a two-level hierarchy (RocksDbStoreBase→RocksDbStore/MergeableRocksDbStore) - Upgrade of RocksDB package from version 9.10.0 to 10.4.2
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/RocksDb.Extensions/RocksDbStoreBase.cs | Refactored from RocksDbStore; now serves as abstract base class for both regular and mergeable stores with common CRUD operations |
| src/RocksDb.Extensions/RocksDbStore.cs | New file wrapping RocksDbStoreBase to maintain backward compatibility; contains the obsolete GetAll() method |
| src/RocksDb.Extensions/MergeableRocksDbStore.cs | New base class for stores requiring merge operations; exposes Merge() method for atomic updates |
| src/RocksDb.Extensions/IMergeOperator.cs | Interface defining FullMerge and PartialMerge operations for custom merge logic |
| src/RocksDb.Extensions/IMergeAccessor.cs | Internal interface extending IRocksDbAccessor with Merge() capability |
| src/RocksDb.Extensions/MergeAccessor.cs | Implementation of merge operations with serialization of keys and operands |
| src/RocksDb.Extensions/MergeOperatorConfig.cs | Internal configuration binding IMergeOperator to RocksDB native merge callbacks |
| src/RocksDb.Extensions/MergeOperators/CollectionOperation.cs | Represents add/remove operations for list merge operator |
| src/RocksDb.Extensions/MergeOperators/OperationType.cs | Enum defining Add and Remove operation types |
| src/RocksDb.Extensions/MergeOperators/ListMergeOperator.cs | Built-in merge operator for atomic list modifications with add/remove support |
| src/RocksDb.Extensions/ListOperationSerializer.cs | Serializes CollectionOperation with optimized formats for primitive vs. complex types |
| src/RocksDb.Extensions/IRocksDbBuilder.cs | Adds AddMergeableStore() method for registering stores with merge operators |
| src/RocksDb.Extensions/RocksDbBuilder.cs | Implements merge store registration with serializer creation for operands |
| src/RocksDb.Extensions/RocksDbContext.cs | Configures merge operators per column family and refactors ColumnFamilyOptions setup |
| src/RocksDb.Extensions/RocksDbAccessor.cs | Changes field visibility to 'private protected' to enable MergeAccessor inheritance; adds WriteOptions to all write operations |
| src/RocksDb.Extensions/RocksDbOptions.cs | Adds internal dictionary for merge operator configurations per column family |
| src/RocksDb.Extensions/RocksDb.Extensions.csproj | Upgrades RocksDB package from 9.10.0.55496 to 10.4.2.63147 |
| test/RocksDb.Extensions.Tests/MergeOperatorTests.cs | Comprehensive test suite covering add, remove, mixed operations, edge cases, and both string and primitive list types |
| README.md | Extensive documentation on merge operators including examples, use cases, and custom implementation guide |
Comments suppressed due to low confidence (2)
src/RocksDb.Extensions/RocksDbStoreBase.cs:14
- The documentation comment states the class is "not intended for direct use by library consumers" and suggests using RocksDbStore or MergeableRocksDbStore instead. However, the class is marked with
[EditorBrowsable(EditorBrowsableState.Never)]but is stillpublic abstract.
For consistency with the stated intent, consider making this class internal rather than public, since it's truly an implementation detail. If it must remain public for technical reasons (e.g., inheritance), the current approach with EditorBrowsable is acceptable, but document why it must be public in a comment.
src/RocksDb.Extensions/RocksDbStoreBase.cs:25
- The constructor uses
protected internalaccess modifier. This allows both derived classes (protected) and classes in the same assembly (internal) to call it. Since RocksDbStore and MergeableRocksDbStore are in the same assembly,protectedalone would be sufficient.
Consider whether protected internal is intentional (e.g., for test projects or extensibility), or if protected would be more appropriate to limit access scope.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f3f7ffa to
1e2fbac
Compare
1e2fbac to
42c6e18
Compare
No description provided.