feat(bitop): Support DIFF, DIFF1, ANDOR, and ONE for command BITOP #3134
Conversation
xuzifu666
changed the title
xuzifu666
changed the title
|
Could you format the code via See https://kvrocks.apache.org/community/contributing for more information. |
|
@xuzifu666 The CI is now using the version clang-format 14, which behaves differently from clang-format > 14 in some ways. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for four new bitwise operations (DIFF, DIFF1, ANDOR, and ONE) to the BITOP command in KvRocks. The implementation includes the core bitwise logic for these operations and corresponding test coverage.
Key changes:
- Added new BitOpFlags enum values for the four new operations
- Implemented optimized 64-bit processing logic for the new operations in the bitmap module
- Added command parsing support for the new operation names
- Updated go-redis dependency to support the new operations in tests
Reviewed Changes
Copilot reviewed 10 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/redis_bitmap.h | Added new enum values for DIFF, DIFF1, ANDOR, and ONE operations |
| src/types/redis_bitmap.cc | Implemented core bitwise logic for new operations with optimized processing |
| src/commands/cmd_bit.cc | Added command parsing support for new operation names |
| tests/gocase/unit/type/bitmap/bitmap_test.go | Added test coverage for the new bitwise operations |
| tests/gocase/go.mod | Updated go-redis dependency from v9.9.0 to v9.12.0 |
| src/search/value.h | Code formatting change (line break adjustment) |
| src/search/interval.h | Code formatting change (line break adjustment) |
| src/config/config.cc | Code formatting change (line break adjustment) |
| src/common/string_util.h | Code formatting change (line break adjustment) |
| src/commands/commander.cc | Code formatting change (line break adjustment) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| uint8_t output = 0, byte = 0, disjunction = 0, common_bits = 0; | ||
| for (; j < frag_maxlen; j++) { |
There was a problem hiding this comment.
The disjunction and common_bits variables are declared outside the loop but should be reset for each byte position. Currently, values from previous iterations will incorrectly influence subsequent calculations for DIFF, DIFF1, ANDOR, and ONE operations.
| uint8_t output = 0, byte = 0, disjunction = 0, common_bits = 0; | |
| for (; j < frag_maxlen; j++) { | |
| uint8_t output = 0, byte = 0; | |
| for (; j < frag_maxlen; j++) { | |
| uint8_t disjunction = 0, common_bits = 0; |
Copilot uses AI. Check for mistakes.
Thansk for reminder and help, I had changed the clang-format version and code format seems OK now. @git-hulk @PragmaTwice |
| Set2SetBit(t, rdb, ctx, "b", []byte("\x01\x02\xff")) | ||
| Set2SetBit(t, rdb, ctx, "c", []byte("\x01\x02\xff")) | ||
| require.NoError(t, rdb.BitOpDiff(ctx, "res1", "a", "b", "c").Err()) | ||
| require.NoError(t, rdb.BitOpDiff1(ctx, "res2", "a", "b", "c").Err()) |
There was a problem hiding this comment.
For the diff1, the result bits should be set in one or more of b or c, but not in a. So the result should be \x00\x00\x00? because a, b, c are the same.
There was a problem hiding this comment.
Yes, it should be 0 according to the definition. I transplanted it completely according to the implementation of Redis. I will check the original logic again.
|
issue: #3132