Support BIT and BYTE options in the BITCOUNT command #2087
Conversation
You can use |
PragmaTwice
changed the title
PragmaTwice
changed the title
|
It looks like clang-tidy run failed. You need to fix it before merging. |
|
@jihuayu Thank you for review carefully, I will fix it when I am free ～ |
tests/cppunit/types/bitmap_test.cc
Outdated
| std::string str = "hello"; | ||
| string_->Set(key_, str); |
There was a problem hiding this comment.
we can avoid hackings for str = "hello", just set specific bits. Setting a string would be hacking
#2075 This patch unify their behaviors
|
Others LGTM except #2087 (comment) |
src/types/redis_bitmap.cc
Outdated
| } | ||
| } | ||
| int64_t bytes = 0; | ||
| if (stop_in_segment >= start_in_segment) { |
There was a problem hiding this comment.
This might visit undefined memory, which could introduce bug
|
@kay011 I found adding the test would find Bitmap has a bug here(BitmapString is ok). I've a fixing for that. Would you mind fix it yourself or let me fix that? |
you can fix it and I will merge it. I think maybe the origin BITCOUNT which used BYTE index also has this bug |
Yeah |
src/types/redis_bitmap.cc
Outdated
| int64_t start_in_segment = 0; // start_index int 1024 bytes segment | ||
| auto stop_in_segment = static_cast<int64_t>(pin_value.size() - 1); // stop_index int 1024 bytes segment | ||
| if (i == start_index) { | ||
| start_in_segment = u_start % kBitmapSegmentBytes; | ||
| if (is_bit_index && start_in_segment <= stop_in_segment && first_byte_neg_mask != 0) { | ||
| uint8_t first_mask_byte = | ||
| kBitSwapTable[static_cast<uint8_t>(pin_value[start_in_segment])] & first_byte_neg_mask; | ||
| mask_cnt += BitmapString::RawPopcount(&first_mask_byte, 1); | ||
| } | ||
| } | ||
| if (i == stop_index) { | ||
| stop_in_segment = u_stop % kBitmapSegmentBytes; | ||
| if (is_bit_index && start_in_segment <= stop_in_segment && last_byte_neg_mask != 0) { | ||
| uint8_t last_mask_byte = kBitSwapTable[static_cast<uint8_t>(pin_value[stop_in_segment])] & last_byte_neg_mask; | ||
| mask_cnt += BitmapString::RawPopcount(&last_mask_byte, 1); | ||
| } | ||
| } | ||
| int64_t bytes = 0; | ||
| if (stop_in_segment >= start_in_segment) { | ||
| bytes = stop_in_segment - start_in_segment + 1; | ||
| } | ||
| *cnt += BitmapString::RawPopcount(reinterpret_cast<const uint8_t *>(pin_value.data()) + start_in_segment, bytes); |
There was a problem hiding this comment.
| int64_t start_in_segment = 0; // start_index int 1024 bytes segment | |
| auto stop_in_segment = static_cast<int64_t>(pin_value.size() - 1); // stop_index int 1024 bytes segment | |
| if (i == start_index) { | |
| start_in_segment = u_start % kBitmapSegmentBytes; | |
| if (is_bit_index && start_in_segment <= stop_in_segment && first_byte_neg_mask != 0) { | |
| uint8_t first_mask_byte = | |
| kBitSwapTable[static_cast<uint8_t>(pin_value[start_in_segment])] & first_byte_neg_mask; | |
| mask_cnt += BitmapString::RawPopcount(&first_mask_byte, 1); | |
| } | |
| } | |
| if (i == stop_index) { | |
| stop_in_segment = u_stop % kBitmapSegmentBytes; | |
| if (is_bit_index && start_in_segment <= stop_in_segment && last_byte_neg_mask != 0) { | |
| uint8_t last_mask_byte = kBitSwapTable[static_cast<uint8_t>(pin_value[stop_in_segment])] & last_byte_neg_mask; | |
| mask_cnt += BitmapString::RawPopcount(&last_mask_byte, 1); | |
| } | |
| } | |
| int64_t bytes = 0; | |
| if (stop_in_segment >= start_in_segment) { | |
| bytes = stop_in_segment - start_in_segment + 1; | |
| } | |
| *cnt += BitmapString::RawPopcount(reinterpret_cast<const uint8_t *>(pin_value.data()) + start_in_segment, bytes); | |
| // Counting bits in [start_in_segment, stop_in_segment] | |
| int64_t start_in_segment = 0; // start_index int 1024 bytes segment | |
| auto readable_stop_in_segment = static_cast<int64_t>(pin_value.size() - 1); // stop_index int 1024 bytes segment | |
| auto stop_in_segment = readable_stop_in_segment; | |
| if (i == start_index) { | |
| start_in_segment = u_start % kBitmapSegmentBytes; | |
| if (is_bit_index && start_in_segment <= readable_stop_in_segment && first_byte_neg_mask != 0) { | |
| uint8_t first_mask_byte = | |
| kBitSwapTable[static_cast<uint8_t>(pin_value[start_in_segment])] & first_byte_neg_mask; | |
| mask_cnt += BitmapString::RawPopcount(&first_mask_byte, 1); | |
| } | |
| } | |
| if (i == stop_index) { | |
| stop_in_segment = u_stop % kBitmapSegmentBytes; | |
| if (is_bit_index && stop_in_segment <= readable_stop_in_segment && last_byte_neg_mask != 0) { | |
| uint8_t last_mask_byte = kBitSwapTable[static_cast<uint8_t>(pin_value[stop_in_segment])] & last_byte_neg_mask; | |
| mask_cnt += BitmapString::RawPopcount(&last_mask_byte, 1); | |
| } | |
| } | |
| if (stop_in_segment >= start_in_segment && readable_stop_in_segment >= start_in_segment) { | |
| int64_t bytes = 0; | |
| bytes = std::min(stop_in_segment, readable_stop_in_segment) - start_in_segment + 1; | |
| *cnt += BitmapString::RawPopcount(reinterpret_cast<const uint8_t *>(pin_value.data()) + start_in_segment, bytes); | |
| } |
|
@kay011 My edits are listed above |
|
|
|
Will wait before Friday to waiting others review it, since I'm one of the editor. |
Co-authored-by: mwish <[email protected]> Co-authored-by: hulk <[email protected]> Co-authored-by: 纪华裕 <[email protected]>
For #2006, This PR enables the BITCOUNT command to support bit and byte options, but the go-redis library does not seem to support this option yet, so only the cpp unit tests are added, and the correctness is verified locally through redis-cli.