mapleFU commented on code in PR #2170:
URL: https://github.com/apache/kvrocks/pull/2170#discussion_r1527060405
##########
src/types/redis_bitmap.cc:
##########
@@ -317,9 +317,13 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool
bit, int64_t start, i
if (metadata.Type() == kRedisString) {
redis::BitmapString bitmap_string_db(storage_, namespace_);
- return bitmap_string_db.BitPos(raw_value, bit, start, stop, stop_given,
pos);
+ return bitmap_string_db.BitPos(raw_value, bit, start, stop, stop_given,
pos, is_bit_index);
}
- std::tie(start, stop) = BitmapString::NormalizeRange(start, stop,
static_cast<int64_t>(metadata.size));
+
+ uint32_t to_bit_factor = is_bit_index ? 8 : 1;
+ auto size = static_cast<int64_t>(metadata.size) *
static_cast<int64_t>(to_bit_factor);
+
+ std::tie(start, stop) = BitmapString::NormalizeRange(start, stop, size);
auto u_start = static_cast<uint32_t>(start);
Review Comment:
would we change uint32 to int64_t here? 🤔
##########
tests/cppunit/types/bitmap_test.cc:
##########
@@ -201,7 +201,7 @@ TEST_P(RedisBitmapTest, BitPosSetBit) {
int64_t pos = 0;
int start_indexes[] = {0, 1, 124, 1025, 1027, 3 * 1024 + 1};
for (size_t i = 0; i < sizeof(start_indexes) / sizeof(start_indexes[0]);
i++) {
- bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos);
+ bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, false);
Review Comment:
Nit: you can use `, /*bit=*/false);` to help future reading
##########
src/types/redis_bitmap.cc:
##########
@@ -359,17 +380,42 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key,
bool bit, int64_t start, i
continue;
}
size_t byte_pos_in_segment = 0;
- if (i == start_index) byte_pos_in_segment = u_start % kBitmapSegmentBytes;
+ size_t byte_with_bit_start = -1;
+ size_t byte_with_bit_stop = -2;
+ // if bit index, (Eg start = 1, stop = 35), then
+ // byte_pos_in_segment should be calculated in bytes, hence divide by 8
+ if (i == start_index) {
+ byte_pos_in_segment = (u_start / to_bit_factor) % kBitmapSegmentBytes;
+ byte_with_bit_start = byte_pos_in_segment;
+ }
size_t stop_byte_in_segment = pin_value.size();
if (i == stop_index) {
- DCHECK_LE(u_stop % kBitmapSegmentBytes + 1, pin_value.size());
- stop_byte_in_segment = u_stop % kBitmapSegmentBytes + 1;
+ DCHECK_LE((u_stop / to_bit_factor) % kBitmapSegmentBytes + 1,
pin_value.size());
+ stop_byte_in_segment = (u_stop / to_bit_factor) % kBitmapSegmentBytes +
1;
+ byte_with_bit_stop = stop_byte_in_segment;
}
// Invariant:
// 1. pin_value.size() <= kBitmapSegmentBytes.
// 2. If it's the last segment, metadata.size % kBitmapSegmentBytes <=
pin_value.size().
for (; byte_pos_in_segment < stop_byte_in_segment; byte_pos_in_segment++) {
- int bit_pos_in_byte_value =
bit_pos_in_byte(pin_value[byte_pos_in_segment], bit);
+ int bit_pos_in_byte_value = -1;
+ if (is_bit_index) {
Review Comment:
This logic looks ok to me, however, should we extract a "begin-end-in-byte"
lambda or function here? It could make code more readable. 🤔
E.g.
```
auto [begin_in_byte, end_in_byte] = range_in_byte(...);
bit_pos_in_byte_startstop(pin_value[..], begin_in_byte, end_in_byte..)
```
##########
src/types/redis_bitmap.cc:
##########
@@ -335,11 +339,28 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key,
bool bit, int64_t start, i
return -1;
};
+ auto bit_pos_in_byte_startstop = [](char byte, bool bit, uint32_t start,
uint32_t stop) -> int {
+ for (uint32_t i = start; i <= stop; i++) {
+ if (bit && (byte & (1 << i)) != 0) return (int)i; // typecast to int
since the value ranges from 0 to 7
+ if (!bit && (byte & (1 << i)) == 0) return (int)i;
+ }
+ return -1;
+ };
+
LatestSnapShot ss(storage_);
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
- uint32_t start_index = u_start / kBitmapSegmentBytes;
- uint32_t stop_index = u_stop / kBitmapSegmentBytes;
+ // if bit index, (Eg start = 1, stop = 35), then
+ // u_start = 1/8 = 0, u_stop = 35/8 = 4 (in bytes)
+ uint32_t start_index = (u_start / to_bit_factor) / kBitmapSegmentBytes;
Review Comment:
The origin code looks like this(my bad), but could we better rename
`start_index` to `start_segment_index`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
