fix: Change the maximum range of setbit's offset to fit redis #2995
Conversation
WalkthroughThis pull request introduces significant changes to bit operation handling across multiple files in the Pika project. The primary modifications include upgrading the Changes
Sequence DiagramsequenceDiagram
participant Client
participant BitOperations
participant Storage
Client->>BitOperations: Perform bit operation
BitOperations->>BitOperations: Validate offset (0-32 bits)
BitOperations->>Storage: Request/Modify bit data
Storage-->>BitOperations: Return bit position/result
BitOperations-->>Client: Return operation result
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pika_bit.cc (1)
Line range hint
28-28: Update the comment to reflect the current maximum bit offset.The comment "value no bigger than 2^18" is outdated. Since
kMaxBitOpInputBithas been updated to 32, the comment should reflect that the maximum value is now 2^32.- // value no bigger than 2^18 + // value no bigger than 2^32tests/unit/type/bitops.tcl (1)
145-154: Consider adding more edge cases.While the test covers basic BITCOUNT functionality with large offsets, consider adding:
- Tests for range counting near the maximum offset
- Tests for empty ranges at large offsets
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)include/pika_define.h(1 hunks)src/pika_bit.cc(1 hunks)src/storage/src/redis_strings.cc(2 hunks)tests/integration/string_test.go(1 hunks)tests/unit/type/bitops.tcl(1 hunks)tools/manifest_generator/include/pika_define.h(1 hunks)tools/pika-port/pika_port_3/pika_define.h(1 hunks)tools/pika_migrate/include/pika_define.h(1 hunks)tools/pika_migrate/src/pika_bit.cc(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🔇 Additional comments (14)
tools/pika-port/pika_port_3/pika_define.h (1)
120-120: LGTM! Constant update aligns with Redis compatibility.The increase of
kMaxBitOpInputBitfrom 21 to 32 allows for larger bit offsets, matching Redis's implementation which supports offsets up to 2^32.Let's verify the constant usage across the codebase:
✅ Verification successful
✓ Constant is consistently defined and used across the codebase
The value of
kMaxBitOpInputBit = 32is uniformly defined in all header files and correctly used in bit operation validations throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all occurrences of kMaxBitOpInputBit to ensure consistent updates rg "kMaxBitOpInputBit" -A 1Length of output: 787
tools/pika_migrate/src/pika_bit.cc (2)
30-33: LGTM! Validation check updated for new bit offset limit.The validation check and comment correctly reflect the new maximum value of 2^32 for bit operations.
171-171: LGTM! Integer handling preserves full range.The direct use of
poswithout casting preserves the full range needed for the increased bit offset limit.tools/manifest_generator/include/pika_define.h (1)
295-295: LGTM! Consistent update in manifest generator.The
kMaxBitOpInputBitconstant is correctly updated to 32 to maintain consistency with the main implementation.tools/pika_migrate/include/pika_define.h (1)
416-416: LGTM! Consistent update in migration tool.The
kMaxBitOpInputBitconstant is correctly updated to 32 to maintain consistency across all tools.tests/integration/string_test.go (1)
280-280: LGTM! Comprehensive test coverage for GetRange.The test case thoroughly verifies string range retrieval with:
- Positive indices (0 to 3)
- Negative indices (-3 to -1)
- Full string range (0 to -1)
- Range exceeding string length (10 to 100)
src/storage/src/redis_strings.cc (2)
1069-1074: LGTM! Enhanced GetBitPos to handle larger bit positions.Changed function signature and variable type to int64_t to support the increased maximum bit offset range.
1152-1152: LGTM! Proper offset calculation for large bit positions.Correctly adds the byte offset to the calculated position without risk of integer overflow.
src/pika_bit.cc (2)
244-244: LGTM: Improved type handling for larger bit positions.The removal of the
static_cast<int>is correct as it allows handling larger bit positions, which is consistent with Redis's behavior and the PR's objective.
Line range hint
1-450: Verify Redis compatibility with larger bit offsets.The implementation looks solid and maintains consistency with Redis's behavior. To ensure full compatibility, let's verify the changes against Redis's implementation.
Run the following script to check Redis's documentation and source code for bit operation limits:
include/pika_define.h (1)
369-369: LGTM! Verify dependent code for breaking changes.The change from 21 to 32 bits aligns with Redis's maximum bit offset (2^32-1). This is a breaking change that expands the supported range.
Let's verify all usages of this constant in the codebase:
✅ Verification successful
Change is safe, all dependencies are aligned
The constant is used consistently for bit offset validation across the codebase. Tools already use the value 32, and the validation logic in
src/pika_bit.ccremains compatible with the expanded range.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all occurrences of kMaxBitOpInputBit rg "kMaxBitOpInputBit" -A 2 -B 2Length of output: 1727
tests/unit/type/bitops.tcl (3)
134-143: LGTM! Comprehensive edge case testing.The test properly verifies:
- Setting/getting bit at maximum valid offset (2^32-1)
- Error handling for invalid offset (2^32)
156-161: LGTM! Good boundary testing.The test effectively verifies BITPOS functionality with maximum offset, checking both set (1) and unset (0) bit positions.
163-172: LGTM! Thorough BITOP testing.The test properly verifies bitwise OR operation with bits set at both ends of the valid range (0 and 2^32-1).
issue: #2983
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores