From 815d11f205105f845b3718e7081bca703d163cf3 Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Fri, 15 Mar 2024 01:34:38 +0530 Subject: [PATCH 1/8] Add bit support to BITPOS Signed-off-by: Mohammad Shehar Yaar Tausif --- src/commands/cmd_bit.cc | 8 +++- src/common/bit_util.h | 60 +++++++++++++++++++++++++----- src/types/redis_bitmap.cc | 49 ++++++++++++++++++++++-- src/types/redis_bitmap.h | 3 +- src/types/redis_bitmap_string.cc | 18 ++++++--- src/types/redis_bitmap_string.h | 2 +- tests/cppunit/types/bitmap_test.cc | 22 +++++------ 7 files changed, 129 insertions(+), 33 deletions(-) diff --git a/src/commands/cmd_bit.cc b/src/commands/cmd_bit.cc index 088e0add853..541bc38f691 100644 --- a/src/commands/cmd_bit.cc +++ b/src/commands/cmd_bit.cc @@ -22,6 +22,7 @@ #include "commands/command_parser.h" #include "error_constants.h" #include "server/server.h" +#include "status.h" #include "types/redis_bitmap.h" namespace redis { @@ -171,6 +172,10 @@ class CommandBitPos : public Commander { stop_ = *parse_stop; } + if (args.size() >= 6 && util::EqualICase(args[5], "BIT")) { + is_bit_index_ = true; + } + auto parse_arg = ParseInt(args[2], 10); if (!parse_arg) { return {Status::RedisParseErr, errValueNotInteger}; @@ -189,7 +194,7 @@ class CommandBitPos : public Commander { Status Execute(Server *srv, Connection *conn, std::string *output) override { int64_t pos = 0; redis::Bitmap bitmap_db(srv->storage, conn->GetNamespace()); - auto s = bitmap_db.BitPos(args_[1], bit_, start_, stop_, stop_given_, &pos); + auto s = bitmap_db.BitPos(args_[1], bit_, start_, stop_, stop_given_, &pos, is_bit_index_); if (!s.ok()) return {Status::RedisExecErr, s.ToString()}; *output = redis::Integer(pos); @@ -201,6 +206,7 @@ class CommandBitPos : public Commander { int64_t stop_ = -1; bool bit_ = false; bool stop_given_ = false; + bool is_bit_index_ = false; }; class CommandBitOp : public Commander { diff --git a/src/common/bit_util.h b/src/common/bit_util.h index 9c23d78bb8d..248c34d40a4 100644 --- a/src/common/bit_util.h +++ b/src/common/bit_util.h @@ -94,15 +94,7 @@ static inline void SetBitTo(uint8_t *bits, int64_t i, bool bit_is_set) { bits[i / 8] ^= static_cast(-static_cast(bit_is_set) ^ bits[i / 8]) & kBitmask[i % 8]; } -/* Return the position of the first bit set to one (if 'bit' is 1) or - * zero (if 'bit' is 0) in the bitmap starting at 's' and long 'count' bytes. - * - * The function is guaranteed to return a value >= 0 if 'bit' is 0 since if - * no zero bit is found, it returns count*8 assuming the string is zero - * padded on the right. However if 'bit' is 1 it is possible that there is - * not a single set bit in the bitmap. In this special case -1 is returned. - * */ -inline int64_t RawBitpos(const uint8_t *c, int64_t count, bool bit) { +inline int64_t RawBitposBytes(const uint8_t *c, int64_t count, bool bit) { int64_t res = 0; if (bit) { @@ -144,6 +136,56 @@ inline int64_t RawBitpos(const uint8_t *c, int64_t count, bool bit) { return res; } +/* Return the position of the first bit set to one (if 'bit' is 1) or + * zero (if 'bit' is 0) in the bitmap 's' starting at 'start_bit' and + * ending at 'end_bit' + * + * The function is guaranteed to return a value >= 0 if 'bit' is 0 since if + * no zero bit is found, it returns count*8 assuming the string is zero + * padded on the right. However if 'bit' is 1 it is possible that there is + * not a single set bit in the bitmap. In this special case -1 is returned. + * */ +inline int64_t RawBitpos(const uint8_t *s, int64_t start_bit, int64_t end_bit, bool bit) { + int64_t start_byte = start_bit / 8; + int64_t end_byte = end_bit / 8; + int64_t start_bit_in_byte = start_bit % 8; + int64_t end_bit_in_byte = end_bit % 8; + + // If the range is contained in a single byte + if (start_byte == end_byte) { + for (int64_t i = start_bit_in_byte; i <= end_bit_in_byte; i++) { + if (msb::GetBitFromByte(s[start_byte], i) == bit) { + return i + start_byte * 8; + } + } + + // return count if no bit is found and bit is 0, else return -1 + return bit ? -1 : (end_bit - start_bit + 1); + } + + // Check the start byte + for (int64_t i = start_bit_in_byte; i < 8; i++) { + if (msb::GetBitFromByte(s[start_byte], i) == bit) { + return i + start_byte * 8; + } + } + + // iterate over long bytes in the middle + int64_t res = msb::RawBitposBytes(s + start_byte + 1, end_byte - start_byte - 1, bit); + if (res != -1 && res != (end_byte - start_byte - 1) * 8) { + return res + (start_byte + 1) * 8; + } + + // check the last byte + for (int64_t i = 0; i <= end_bit_in_byte; i++) { + if (msb::GetBitFromByte(s[end_byte], i) == bit) { + return i + end_byte * 8; + } + } + + return bit ? -1 : (end_bit - start_bit + 1); +} + } // namespace msb } // namespace util diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index ac3d8768f8e..806e633a7d2 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -303,7 +303,7 @@ rocksdb::Status Bitmap::BitCount(const Slice &user_key, int64_t start, int64_t s } rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, int64_t stop, bool stop_given, - int64_t *pos) { + int64_t *pos, bool is_bit_index) { std::string raw_value; std::string ns_key = AppendNamespacePrefix(user_key); @@ -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(metadata.size)); + + auto size = static_cast(metadata.size); + auto length = is_bit_index ? size * 8 : size; + + std::tie(start, stop) = BitmapString::NormalizeRange(start, stop, length); auto u_start = static_cast(start); auto u_stop = static_cast(stop); if (u_start > u_stop) { @@ -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; + uint32_t start_bit_pos_in_byte = 0; + uint32_t stop_bit_pos_in_byte = 0; + + if (is_bit_index) { + start_bit_pos_in_byte = u_start % 8; + stop_bit_pos_in_byte = u_stop % 8; + start_index = (u_start / 8) / kBitmapSegmentBytes; + stop_index = (u_stop / 8) / kBitmapSegmentBytes; + } // Don't use multi get to prevent large range query, and take too much memory // Searching bits in segments [start_index, stop_index]. for (uint32_t i = start_index; i <= stop_index; i++) { @@ -369,7 +390,27 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i // 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) { + uint32_t start_byte = start_index / 8; + uint32_t stop_byte = stop_index / 8; + if (start_byte == stop_byte) { + bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, + stop_bit_pos_in_byte); + } else if (i == start_index) { + bit_pos_in_byte_value = + bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, 7); + } else if (i == stop_index) { + bit_pos_in_byte_value = + bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, 0, stop_bit_pos_in_byte); + } else { + bit_pos_in_byte_value = bit_pos_in_byte(pin_value[byte_pos_in_segment], bit); + } + } else { + bit_pos_in_byte_value = bit_pos_in_byte(pin_value[byte_pos_in_segment], bit); + } + if (bit_pos_in_byte_value != -1) { *pos = static_cast(i * kBitmapSegmentBits + byte_pos_in_segment * 8 + bit_pos_in_byte_value); return rocksdb::Status::OK(); diff --git a/src/types/redis_bitmap.h b/src/types/redis_bitmap.h index 349982d8ce6..30a071579b4 100644 --- a/src/types/redis_bitmap.h +++ b/src/types/redis_bitmap.h @@ -50,7 +50,8 @@ class Bitmap : public Database { rocksdb::Status GetString(const Slice &user_key, uint32_t max_btos_size, std::string *value); rocksdb::Status SetBit(const Slice &user_key, uint32_t bit_offset, bool new_bit, bool *old_bit); rocksdb::Status BitCount(const Slice &user_key, int64_t start, int64_t stop, bool is_bit_index, uint32_t *cnt); - rocksdb::Status BitPos(const Slice &user_key, bool bit, int64_t start, int64_t stop, bool stop_given, int64_t *pos); + rocksdb::Status BitPos(const Slice &user_key, bool bit, int64_t start, int64_t stop, bool stop_given, int64_t *pos, + bool is_bit_index); rocksdb::Status BitOp(BitOpFlags op_flag, const std::string &op_name, const Slice &user_key, const std::vector &op_keys, int64_t *len); rocksdb::Status Bitfield(const Slice &user_key, const std::vector &ops, diff --git a/src/types/redis_bitmap_string.cc b/src/types/redis_bitmap_string.cc index b226d9c2f7a..66ca4eafa8d 100644 --- a/src/types/redis_bitmap_string.cc +++ b/src/types/redis_bitmap_string.cc @@ -100,17 +100,24 @@ rocksdb::Status BitmapString::BitCount(const std::string &raw_value, int64_t sta } rocksdb::Status BitmapString::BitPos(const std::string &raw_value, bool bit, int64_t start, int64_t stop, - bool stop_given, int64_t *pos) { + bool stop_given, int64_t *pos, bool is_bit_index) { std::string_view string_value = std::string_view{raw_value}.substr(Metadata::GetOffsetAfterExpire(raw_value[0])); auto strlen = static_cast(string_value.size()); /* Convert negative and out-of-bound indexes */ - std::tie(start, stop) = NormalizeRange(start, stop, strlen); + + int64_t length = is_bit_index ? strlen * 8 : strlen; + std::tie(start, stop) = NormalizeRange(start, stop, length); if (start > stop) { *pos = -1; } else { - int64_t bytes = stop - start + 1; - *pos = util::msb::RawBitpos(reinterpret_cast(string_value.data()) + start, bytes, bit); + if (!is_bit_index) { + start *= 8; + stop = (stop * 8) + 7; + } + + int64_t count_bits = stop - start + 1; + *pos = util::msb::RawBitpos(reinterpret_cast(string_value.data()), start, stop, bit); /* If we are looking for clear bits, and the user specified an exact * range with start-end, we can't consider the right of the range as @@ -119,11 +126,10 @@ rocksdb::Status BitmapString::BitPos(const std::string &raw_value, bool bit, int * So if redisBitpos() returns the first bit outside the range, * we return -1 to the caller, to mean, in the specified range there * is not a single "0" bit. */ - if (stop_given && bit == 0 && *pos == bytes * 8) { + if (stop_given && bit == 0 && *pos == count_bits) { *pos = -1; return rocksdb::Status::OK(); } - if (*pos != -1) *pos += start * 8; /* Adjust for the bytes we skipped. */ } return rocksdb::Status::OK(); } diff --git a/src/types/redis_bitmap_string.h b/src/types/redis_bitmap_string.h index 7997165afa3..030415c3492 100644 --- a/src/types/redis_bitmap_string.h +++ b/src/types/redis_bitmap_string.h @@ -39,7 +39,7 @@ class BitmapString : public Database { static rocksdb::Status BitCount(const std::string &raw_value, int64_t start, int64_t stop, bool is_bit_index, uint32_t *cnt); static rocksdb::Status BitPos(const std::string &raw_value, bool bit, int64_t start, int64_t stop, bool stop_given, - int64_t *pos); + int64_t *pos, bool is_bit_index); rocksdb::Status Bitfield(const Slice &ns_key, std::string *raw_value, const std::vector &ops, std::vector> *rets); static rocksdb::Status BitfieldReadOnly(const Slice &ns_key, const std::string &raw_value, diff --git a/tests/cppunit/types/bitmap_test.cc b/tests/cppunit/types/bitmap_test.cc index 4795e476ad0..d3900ae316f 100644 --- a/tests/cppunit/types/bitmap_test.cc +++ b/tests/cppunit/types/bitmap_test.cc @@ -179,7 +179,7 @@ TEST_P(RedisBitmapTest, BitPosClearBit) { /// /// String will set a empty string value when initializing, so, when first /// querying, it should return -1. - bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos); + bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, false); if (i == 0 && !use_bitmap) { EXPECT_EQ(pos, -1); } else { @@ -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); EXPECT_EQ(pos, offsets[i]); } auto s = bitmap_->Del(key_); @@ -215,19 +215,19 @@ TEST_P(RedisBitmapTest, BitPosNegative) { } int64_t pos = 0; // First bit is negative - bitmap_->BitPos(key_, false, 0, -1, true, &pos); + bitmap_->BitPos(key_, false, 0, -1, true, &pos, false); EXPECT_EQ(0, pos); // 8 * 1024 - 1 bit is positive - bitmap_->BitPos(key_, true, 0, -1, true, &pos); + bitmap_->BitPos(key_, true, 0, -1, true, &pos, false); EXPECT_EQ(8 * 1024 - 1, pos); // First bit in 1023 byte is negative - bitmap_->BitPos(key_, false, -1, -1, true, &pos); + bitmap_->BitPos(key_, false, -1, -1, true, &pos, false); EXPECT_EQ(8 * 1023, pos); // Last Bit in 1023 byte is positive - bitmap_->BitPos(key_, true, -1, -1, true, &pos); + bitmap_->BitPos(key_, true, -1, -1, true, &pos, false); EXPECT_EQ(8 * 1024 - 1, pos); // Large negative number will be normalized. - bitmap_->BitPos(key_, false, -10000, -10000, true, &pos); + bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, false); EXPECT_EQ(0, pos); auto s = bitmap_->Del(key_); @@ -242,9 +242,9 @@ TEST_P(RedisBitmapTest, BitPosStopGiven) { EXPECT_FALSE(bit); } int64_t pos = 0; - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, false); EXPECT_EQ(-1, pos); - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/false, &pos); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/false, &pos, false); EXPECT_EQ(8, pos); // Set a bit at 8 not affect that @@ -253,9 +253,9 @@ TEST_P(RedisBitmapTest, BitPosStopGiven) { bitmap_->SetBit(key_, 8, true, &bit); EXPECT_FALSE(bit); } - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, false); EXPECT_EQ(-1, pos); - bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos); + bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos, false); EXPECT_EQ(9, pos); auto s = bitmap_->Del(key_); From f0f7dbb3ece133b31283fd3aab47bfeafad254c6 Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Sat, 16 Mar 2024 02:35:14 +0530 Subject: [PATCH 2/8] Fix bitmap logic and add test cases Signed-off-by: Mohammad Shehar Yaar Tausif --- src/types/redis_bitmap.cc | 37 +++++----- tests/cppunit/types/bitmap_test.cc | 71 ++++++++++++++++++++ tests/gocase/unit/type/bitmap/bitmap_test.go | 21 ++++++ 3 files changed, 113 insertions(+), 16 deletions(-) diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index 806e633a7d2..ac507c34ff9 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -320,10 +320,10 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i return bitmap_string_db.BitPos(raw_value, bit, start, stop, stop_given, pos, is_bit_index); } - auto size = static_cast(metadata.size); - auto length = is_bit_index ? size * 8 : size; + uint32_t to_bit_factor = is_bit_index ? 8 : 1; + auto size = static_cast(metadata.size) * static_cast(to_bit_factor); - std::tie(start, stop) = BitmapString::NormalizeRange(start, stop, length); + std::tie(start, stop) = BitmapString::NormalizeRange(start, stop, size); auto u_start = static_cast(start); auto u_stop = static_cast(stop); if (u_start > u_stop) { @@ -350,16 +350,16 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i 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; + uint32_t stop_index = (u_stop / to_bit_factor) / kBitmapSegmentBytes; uint32_t start_bit_pos_in_byte = 0; uint32_t stop_bit_pos_in_byte = 0; if (is_bit_index) { start_bit_pos_in_byte = u_start % 8; stop_bit_pos_in_byte = u_stop % 8; - start_index = (u_start / 8) / kBitmapSegmentBytes; - stop_index = (u_stop / 8) / kBitmapSegmentBytes; } // Don't use multi get to prevent large range query, and take too much memory // Searching bits in segments [start_index, stop_index]. @@ -380,28 +380,33 @@ 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 = -1; - if (is_bit_index) { - uint32_t start_byte = start_index / 8; - uint32_t stop_byte = stop_index / 8; - if (start_byte == stop_byte) { + if (byte_with_bit_start == byte_with_bit_stop && byte_pos_in_segment == byte_with_bit_start) { bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, stop_bit_pos_in_byte); - } else if (i == start_index) { + } else if (byte_pos_in_segment == byte_with_bit_start) { bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, 7); - } else if (i == stop_index) { + } else if (byte_pos_in_segment == byte_with_bit_stop) { bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, 0, stop_bit_pos_in_byte); } else { diff --git a/tests/cppunit/types/bitmap_test.cc b/tests/cppunit/types/bitmap_test.cc index d3900ae316f..8baa7a9258a 100644 --- a/tests/cppunit/types/bitmap_test.cc +++ b/tests/cppunit/types/bitmap_test.cc @@ -625,3 +625,74 @@ TEST_P(RedisBitmapTest, BitfieldStringGetSetTest) { i += 8; } } + +TEST_P(RedisBitmapTest, BitPosClearBit_BIT) { + int64_t pos = 0; + bool old_bit = false; + bool use_bitmap = GetParam(); + for (int i = 0; i < 1024 + 16; i++) { + /// ``` + /// redis> set k1 "" + /// "OK" + /// redis> bitpos k1 0 + /// (integer) -1 + /// redis> bitpos k2 0 + /// (integer) 0 + /// ``` + /// + /// String will set a empty string value when initializing, so, when first + /// querying, it should return -1. + bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, true); + if (i == 0 && !use_bitmap) { + EXPECT_EQ(pos, -1); + } else { + EXPECT_EQ(pos, i); + } + + bitmap_->SetBit(key_, i, true, &old_bit); + EXPECT_FALSE(old_bit); + } + auto s = bitmap_->Del(key_); +} + +TEST_P(RedisBitmapTest, BitPosSetBit_BIT) { + uint32_t offsets[] = {0, 123, 1024 * 8, 1024 * 8 + 16, 3 * 1024 * 8, 3 * 1024 * 8 + 16}; + for (const auto &offset : offsets) { + bool bit = false; + bitmap_->SetBit(key_, offset, true, &bit); + } + int64_t pos = 0; + int start_indexes[] = {0, 1 * 8, 124 * 8, 1025 * 8, 1027 * 8, (3 * 1024 + 1) * 8}; + for (size_t i = 0; i < sizeof(start_indexes) / sizeof(start_indexes[0]); i++) { + bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, true); + EXPECT_EQ(pos, offsets[i]); + } + auto s = bitmap_->Del(key_); +} + +TEST_P(RedisBitmapTest, BitPosNegative_BIT) { + { + bool bit = false; + bitmap_->SetBit(key_, 8 * 1024 - 1, true, &bit); + EXPECT_FALSE(bit); + } + int64_t pos = 0; + // First bit is negative + bitmap_->BitPos(key_, false, 0, -1, true, &pos, true); + EXPECT_EQ(0, pos); + // 8 * 1024 - 1 bit is positive + bitmap_->BitPos(key_, true, 0, -1, true, &pos, true); + EXPECT_EQ(8 * 1024 - 1, pos); + // For bit indexing this should evalue to -1 instead of 8 * 1023 (in the case of byte index) + // This matches the behaviour on redis side + bitmap_->BitPos(key_, false, -1, -1, true, &pos, true); + EXPECT_EQ(-1, pos); + // Last Bit in 1023 byte is positive + bitmap_->BitPos(key_, true, -1, -1, true, &pos, true); + EXPECT_EQ(8 * 1024 - 1, pos); + // Large negative number will be normalized. + bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, true); + EXPECT_EQ(0, pos); + + auto s = bitmap_->Del(key_); +} diff --git a/tests/gocase/unit/type/bitmap/bitmap_test.go b/tests/gocase/unit/type/bitmap/bitmap_test.go index 508f52dd82b..0b6e8fd597f 100644 --- a/tests/gocase/unit/type/bitmap/bitmap_test.go +++ b/tests/gocase/unit/type/bitmap/bitmap_test.go @@ -378,4 +378,25 @@ func TestBitmap(t *testing.T) { require.EqualValues(t, 't', res.Val()[0]) require.ErrorContains(t, rdb.Do(ctx, "BITFIELD_RO", "str", "INCRBY", "u8", "32", 2).Err(), "BITFIELD_RO only supports the GET subcommand") }) + + t.Run("BITPOS BIT option check", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "mykey", "\x00\xff\xf0", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "mykey", 1, 7, 15, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 8, cmd.Val()) + }) + + t.Run("BITPOS BIT not found check check", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "mykey", "\x00\xff\xf0", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "mykey", 0, 0, 5, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 0, cmd.Val()) + }) + + t.Run("BITPOS BIT not found check check", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "mykey", "\x00\xff\xf0", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "mykey", 0, 2, 3, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 2, cmd.Val()) + }) } From 5b86fffa5786eb01627c4b7942a8606a265ffe04 Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Sat, 16 Mar 2024 15:36:20 +0530 Subject: [PATCH 3/8] Add lamda for bit range query Signed-off-by: Mohammad Shehar Yaar Tausif --- src/types/redis_bitmap.cc | 38 ++++++++++++++++-------------- tests/cppunit/types/bitmap_test.cc | 36 ++++++++++++++-------------- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index ac507c34ff9..d682f6da3fc 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -21,7 +21,9 @@ #include "redis_bitmap.h" #include +#include #include +#include #include #include "common/bit_util.h" @@ -347,13 +349,21 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i return -1; }; + auto range_in_byte = [](uint32_t byte_start, uint32_t byte_end, uint32_t curr_byte, uint32_t start_bit, + uint32_t end_bit) -> std::pair { + if (curr_byte == byte_start && curr_byte == byte_end) return {start_bit, end_bit}; + if (curr_byte == byte_start) return {start_bit, 7}; + if (curr_byte == byte_end) return {0, end_bit}; + return {0, 7}; + }; + LatestSnapShot ss(storage_); rocksdb::ReadOptions read_options; read_options.snapshot = ss.GetSnapShot(); // 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; - uint32_t stop_index = (u_stop / to_bit_factor) / kBitmapSegmentBytes; + uint32_t start_segment_index = (u_start / to_bit_factor) / kBitmapSegmentBytes; + uint32_t stop_segment_index = (u_stop / to_bit_factor) / kBitmapSegmentBytes; uint32_t start_bit_pos_in_byte = 0; uint32_t stop_bit_pos_in_byte = 0; @@ -363,7 +373,7 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i } // Don't use multi get to prevent large range query, and take too much memory // Searching bits in segments [start_index, stop_index]. - for (uint32_t i = start_index; i <= stop_index; i++) { + for (uint32_t i = start_segment_index; i <= stop_segment_index; i++) { rocksdb::PinnableSlice pin_value; std::string sub_key = InternalKey(ns_key, std::to_string(i * kBitmapSegmentBytes), metadata.version, storage_->IsSlotIdEncoded()) @@ -384,12 +394,12 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i 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) { + if (i == start_segment_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) { + if (i == stop_segment_index) { 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; @@ -400,18 +410,10 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i for (; byte_pos_in_segment < stop_byte_in_segment; byte_pos_in_segment++) { int bit_pos_in_byte_value = -1; if (is_bit_index) { - if (byte_with_bit_start == byte_with_bit_stop && byte_pos_in_segment == byte_with_bit_start) { - bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, - stop_bit_pos_in_byte); - } else if (byte_pos_in_segment == byte_with_bit_start) { - bit_pos_in_byte_value = - bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit_pos_in_byte, 7); - } else if (byte_pos_in_segment == byte_with_bit_stop) { - bit_pos_in_byte_value = - bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, 0, stop_bit_pos_in_byte); - } else { - bit_pos_in_byte_value = bit_pos_in_byte(pin_value[byte_pos_in_segment], bit); - } + uint32_t start_bit = 0, stop_bit = 7; + std::tie(start_bit, stop_bit) = range_in_byte(byte_with_bit_start, byte_with_bit_stop, byte_pos_in_segment, + start_bit_pos_in_byte, stop_bit_pos_in_byte); + bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit, stop_bit); } else { bit_pos_in_byte_value = bit_pos_in_byte(pin_value[byte_pos_in_segment], bit); } @@ -428,7 +430,7 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i // 1. If it's the last segment, we've done searching in the above loop. // 2. If it's not the last segment, we can check if the segment is all 0. if (pin_value.size() < kBitmapSegmentBytes) { - if (i == stop_index) { + if (i == stop_segment_index) { continue; } *pos = static_cast(i * kBitmapSegmentBits + pin_value.size() * 8); diff --git a/tests/cppunit/types/bitmap_test.cc b/tests/cppunit/types/bitmap_test.cc index 8baa7a9258a..86b14a781f8 100644 --- a/tests/cppunit/types/bitmap_test.cc +++ b/tests/cppunit/types/bitmap_test.cc @@ -179,7 +179,7 @@ TEST_P(RedisBitmapTest, BitPosClearBit) { /// /// String will set a empty string value when initializing, so, when first /// querying, it should return -1. - bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, false); + bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, /*bit_index=*/false); if (i == 0 && !use_bitmap) { EXPECT_EQ(pos, -1); } else { @@ -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, false); + bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, /*bit_index=*/false); EXPECT_EQ(pos, offsets[i]); } auto s = bitmap_->Del(key_); @@ -215,19 +215,19 @@ TEST_P(RedisBitmapTest, BitPosNegative) { } int64_t pos = 0; // First bit is negative - bitmap_->BitPos(key_, false, 0, -1, true, &pos, false); + bitmap_->BitPos(key_, false, 0, -1, true, &pos, /*bit_index=*/false); EXPECT_EQ(0, pos); // 8 * 1024 - 1 bit is positive - bitmap_->BitPos(key_, true, 0, -1, true, &pos, false); + bitmap_->BitPos(key_, true, 0, -1, true, &pos, /*bit_index=*/false); EXPECT_EQ(8 * 1024 - 1, pos); // First bit in 1023 byte is negative - bitmap_->BitPos(key_, false, -1, -1, true, &pos, false); + bitmap_->BitPos(key_, false, -1, -1, true, &pos, /*bit_index=*/false); EXPECT_EQ(8 * 1023, pos); // Last Bit in 1023 byte is positive - bitmap_->BitPos(key_, true, -1, -1, true, &pos, false); + bitmap_->BitPos(key_, true, -1, -1, true, &pos, /*bit_index=*/false); EXPECT_EQ(8 * 1024 - 1, pos); // Large negative number will be normalized. - bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, false); + bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, /*bit_index=*/false); EXPECT_EQ(0, pos); auto s = bitmap_->Del(key_); @@ -242,9 +242,9 @@ TEST_P(RedisBitmapTest, BitPosStopGiven) { EXPECT_FALSE(bit); } int64_t pos = 0; - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, false); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, /*bit_index=*/false); EXPECT_EQ(-1, pos); - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/false, &pos, false); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/false, &pos, /*bit_index=*/false); EXPECT_EQ(8, pos); // Set a bit at 8 not affect that @@ -253,9 +253,9 @@ TEST_P(RedisBitmapTest, BitPosStopGiven) { bitmap_->SetBit(key_, 8, true, &bit); EXPECT_FALSE(bit); } - bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, false); + bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, /*bit_index=*/false); EXPECT_EQ(-1, pos); - bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos, false); + bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos, /*bit_index=*/ false); EXPECT_EQ(9, pos); auto s = bitmap_->Del(key_); @@ -642,7 +642,7 @@ TEST_P(RedisBitmapTest, BitPosClearBit_BIT) { /// /// String will set a empty string value when initializing, so, when first /// querying, it should return -1. - bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, true); + bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, /*bit_index=*/true); if (i == 0 && !use_bitmap) { EXPECT_EQ(pos, -1); } else { @@ -664,7 +664,7 @@ TEST_P(RedisBitmapTest, BitPosSetBit_BIT) { int64_t pos = 0; int start_indexes[] = {0, 1 * 8, 124 * 8, 1025 * 8, 1027 * 8, (3 * 1024 + 1) * 8}; for (size_t i = 0; i < sizeof(start_indexes) / sizeof(start_indexes[0]); i++) { - bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, true); + bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, /*bit_index=*/true); EXPECT_EQ(pos, offsets[i]); } auto s = bitmap_->Del(key_); @@ -678,20 +678,20 @@ TEST_P(RedisBitmapTest, BitPosNegative_BIT) { } int64_t pos = 0; // First bit is negative - bitmap_->BitPos(key_, false, 0, -1, true, &pos, true); + bitmap_->BitPos(key_, false, 0, -1, true, &pos, /*bit_index=*/true); EXPECT_EQ(0, pos); // 8 * 1024 - 1 bit is positive - bitmap_->BitPos(key_, true, 0, -1, true, &pos, true); + bitmap_->BitPos(key_, true, 0, -1, true, &pos, /*bit_index=*/true); EXPECT_EQ(8 * 1024 - 1, pos); // For bit indexing this should evalue to -1 instead of 8 * 1023 (in the case of byte index) // This matches the behaviour on redis side - bitmap_->BitPos(key_, false, -1, -1, true, &pos, true); + bitmap_->BitPos(key_, false, -1, -1, true, &pos, /*bit_index=*/true); EXPECT_EQ(-1, pos); // Last Bit in 1023 byte is positive - bitmap_->BitPos(key_, true, -1, -1, true, &pos, true); + bitmap_->BitPos(key_, true, -1, -1, true, &pos, /*bit_index=*/true); EXPECT_EQ(8 * 1024 - 1, pos); // Large negative number will be normalized. - bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, true); + bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, /*bit_index=*/true); EXPECT_EQ(0, pos); auto s = bitmap_->Del(key_); From d64ec7b04ddee9d664b906cafd2bddcc9ca25004 Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Sat, 16 Mar 2024 20:20:28 +0530 Subject: [PATCH 4/8] Fix clang format Signed-off-by: Mohammad Shehar Yaar Tausif --- src/common/status.h | 2 +- tests/cppunit/types/bitmap_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/status.h b/src/common/status.h index b425ea5b238..7eb0050d561 100644 --- a/src/common/status.h +++ b/src/common/status.h @@ -163,7 +163,7 @@ struct StringInStatusOr> : StringInStatusOr(StringInStatusOr&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT template ::inplace, int> = 0> StringInStatusOr(StringInStatusOr&& v) // NOLINT - : BaseType((typename StringInStatusOr::BaseType &&)(std::move(v))) {} + : BaseType((typename StringInStatusOr::BaseType&&)(std::move(v))) {} StringInStatusOr(const StringInStatusOr& v) = delete; diff --git a/tests/cppunit/types/bitmap_test.cc b/tests/cppunit/types/bitmap_test.cc index 86b14a781f8..281c3fb95d6 100644 --- a/tests/cppunit/types/bitmap_test.cc +++ b/tests/cppunit/types/bitmap_test.cc @@ -255,7 +255,7 @@ TEST_P(RedisBitmapTest, BitPosStopGiven) { } bitmap_->BitPos(key_, false, 0, 0, /*stop_given=*/true, &pos, /*bit_index=*/false); EXPECT_EQ(-1, pos); - bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos, /*bit_index=*/ false); + bitmap_->BitPos(key_, false, 0, 1, /*stop_given=*/false, &pos, /*bit_index=*/false); EXPECT_EQ(9, pos); auto s = bitmap_->Del(key_); From 7ca036e275e27a45b108ae30aa05aeefb33b861f Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Sun, 17 Mar 2024 18:57:57 +0530 Subject: [PATCH 5/8] Fix CI and fix RawBitpos Signed-off-by: Mohammad Shehar Yaar Tausif --- src/common/bit_util.h | 64 ++----- src/common/status.h | 2 +- src/types/redis_bitmap_string.cc | 73 ++++++-- tests/cppunit/types/bitmap_test.cc | 71 -------- tests/gocase/unit/type/bitmap/bitmap_test.go | 169 +++++++++++++++++++ 5 files changed, 241 insertions(+), 138 deletions(-) diff --git a/src/common/bit_util.h b/src/common/bit_util.h index 248c34d40a4..898e3cdd7fb 100644 --- a/src/common/bit_util.h +++ b/src/common/bit_util.h @@ -94,7 +94,19 @@ static inline void SetBitTo(uint8_t *bits, int64_t i, bool bit_is_set) { bits[i / 8] ^= static_cast(-static_cast(bit_is_set) ^ bits[i / 8]) & kBitmask[i % 8]; } -inline int64_t RawBitposBytes(const uint8_t *c, int64_t count, bool bit) { +/* Return the position of the first bit set to one (if 'bit' is 1) or + * zero (if 'bit' is 0) in the bitmap starting at 's' and long 'count' bytes. + * + * The function is guaranteed to return a value >= 0 if 'bit' is 0 since if + * no zero bit is found, it returns count*8 assuming the string is zero + * padded on the right. However if 'bit' is 1 it is possible that there is + * not a single set bit in the bitmap. In this special case -1 is returned. + * */ +inline int64_t RawBitpos(const uint8_t *c, int64_t count, bool bit) { + if (count == 0) { + return -1; + } + int64_t res = 0; if (bit) { @@ -136,56 +148,6 @@ inline int64_t RawBitposBytes(const uint8_t *c, int64_t count, bool bit) { return res; } -/* Return the position of the first bit set to one (if 'bit' is 1) or - * zero (if 'bit' is 0) in the bitmap 's' starting at 'start_bit' and - * ending at 'end_bit' - * - * The function is guaranteed to return a value >= 0 if 'bit' is 0 since if - * no zero bit is found, it returns count*8 assuming the string is zero - * padded on the right. However if 'bit' is 1 it is possible that there is - * not a single set bit in the bitmap. In this special case -1 is returned. - * */ -inline int64_t RawBitpos(const uint8_t *s, int64_t start_bit, int64_t end_bit, bool bit) { - int64_t start_byte = start_bit / 8; - int64_t end_byte = end_bit / 8; - int64_t start_bit_in_byte = start_bit % 8; - int64_t end_bit_in_byte = end_bit % 8; - - // If the range is contained in a single byte - if (start_byte == end_byte) { - for (int64_t i = start_bit_in_byte; i <= end_bit_in_byte; i++) { - if (msb::GetBitFromByte(s[start_byte], i) == bit) { - return i + start_byte * 8; - } - } - - // return count if no bit is found and bit is 0, else return -1 - return bit ? -1 : (end_bit - start_bit + 1); - } - - // Check the start byte - for (int64_t i = start_bit_in_byte; i < 8; i++) { - if (msb::GetBitFromByte(s[start_byte], i) == bit) { - return i + start_byte * 8; - } - } - - // iterate over long bytes in the middle - int64_t res = msb::RawBitposBytes(s + start_byte + 1, end_byte - start_byte - 1, bit); - if (res != -1 && res != (end_byte - start_byte - 1) * 8) { - return res + (start_byte + 1) * 8; - } - - // check the last byte - for (int64_t i = 0; i <= end_bit_in_byte; i++) { - if (msb::GetBitFromByte(s[end_byte], i) == bit) { - return i + end_byte * 8; - } - } - - return bit ? -1 : (end_bit - start_bit + 1); -} - } // namespace msb } // namespace util diff --git a/src/common/status.h b/src/common/status.h index 7eb0050d561..b425ea5b238 100644 --- a/src/common/status.h +++ b/src/common/status.h @@ -163,7 +163,7 @@ struct StringInStatusOr> : StringInStatusOr(StringInStatusOr&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT template ::inplace, int> = 0> StringInStatusOr(StringInStatusOr&& v) // NOLINT - : BaseType((typename StringInStatusOr::BaseType&&)(std::move(v))) {} + : BaseType((typename StringInStatusOr::BaseType &&)(std::move(v))) {} StringInStatusOr(const StringInStatusOr& v) = delete; diff --git a/src/types/redis_bitmap_string.cc b/src/types/redis_bitmap_string.cc index 66ca4eafa8d..b10a5d45d49 100644 --- a/src/types/redis_bitmap_string.cc +++ b/src/types/redis_bitmap_string.cc @@ -110,27 +110,70 @@ rocksdb::Status BitmapString::BitPos(const std::string &raw_value, bool bit, int if (start > stop) { *pos = -1; - } else { - if (!is_bit_index) { - start *= 8; - stop = (stop * 8) + 7; + return rocksdb::Status::OK(); + } + + int64_t byte_start = is_bit_index ? start / 8 : start; + int64_t byte_stop = is_bit_index ? stop / 8 : stop; + int64_t bit_in_start_byte = is_bit_index ? start % 8 : 0; + int64_t bit_in_stop_byte = is_bit_index ? stop % 8 : 7; + int64_t bytes_cnt = byte_stop - byte_start + 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 (util::msb::GetBitFromByte(byte, i) == bit) { + return (int)i; + } } + return -1; + }; + + // if the bit start and bit end are in the same byte, we can process it manually + if (is_bit_index && byte_start == byte_stop) { + int res = bit_pos_in_byte_startstop(string_value[byte_start], bit, bit_in_start_byte, bit_in_stop_byte); + if (res != -1) { + *pos = res + byte_start * 8; + return rocksdb::Status::OK(); + } + *pos = -1; + return rocksdb::Status::OK(); + } - int64_t count_bits = stop - start + 1; - *pos = util::msb::RawBitpos(reinterpret_cast(string_value.data()), start, stop, bit); - - /* If we are looking for clear bits, and the user specified an exact - * range with start-end, we can't consider the right of the range as - * zero padded (as we do when no explicit end is given). - * - * So if redisBitpos() returns the first bit outside the range, - * we return -1 to the caller, to mean, in the specified range there - * is not a single "0" bit. */ - if (stop_given && bit == 0 && *pos == count_bits) { + if (is_bit_index && bit_in_start_byte != 0) { + // process first byte + int res = bit_pos_in_byte_startstop(string_value[byte_start], bit, bit_in_start_byte, 7); + if (res != -1) { + *pos = res + byte_start * 8; + return rocksdb::Status::OK(); + } + + byte_start++; + bytes_cnt--; + } + + *pos = util::msb::RawBitpos(reinterpret_cast(string_value.data()) + byte_start, bytes_cnt, bit); + + if (is_bit_index && *pos != -1 && *pos != bytes_cnt * 8) { + // if the pos is more than stop bit, then it is not in the range + if (*pos > stop) { *pos = -1; return rocksdb::Status::OK(); } } + + /* If we are looking for clear bits, and the user specified an exact + * range with start-end, we tcan' consider the right of the range as + * zero padded (as we do when no explicit end is given). + * + * So if redisBitpos() returns the first bit outside the range, + * we return -1 to the caller, to mean, in the specified range there + * is not a single "0" bit. */ + if (stop_given && bit == 0 && *pos == bytes_cnt * 8) { + *pos = -1; + return rocksdb::Status::OK(); + } + if (*pos != -1) *pos += byte_start * 8; /* Adjust for the bytes we skipped. */ + return rocksdb::Status::OK(); } diff --git a/tests/cppunit/types/bitmap_test.cc b/tests/cppunit/types/bitmap_test.cc index 281c3fb95d6..6ec2d9e39a1 100644 --- a/tests/cppunit/types/bitmap_test.cc +++ b/tests/cppunit/types/bitmap_test.cc @@ -625,74 +625,3 @@ TEST_P(RedisBitmapTest, BitfieldStringGetSetTest) { i += 8; } } - -TEST_P(RedisBitmapTest, BitPosClearBit_BIT) { - int64_t pos = 0; - bool old_bit = false; - bool use_bitmap = GetParam(); - for (int i = 0; i < 1024 + 16; i++) { - /// ``` - /// redis> set k1 "" - /// "OK" - /// redis> bitpos k1 0 - /// (integer) -1 - /// redis> bitpos k2 0 - /// (integer) 0 - /// ``` - /// - /// String will set a empty string value when initializing, so, when first - /// querying, it should return -1. - bitmap_->BitPos(key_, false, 0, -1, /*stop_given=*/false, &pos, /*bit_index=*/true); - if (i == 0 && !use_bitmap) { - EXPECT_EQ(pos, -1); - } else { - EXPECT_EQ(pos, i); - } - - bitmap_->SetBit(key_, i, true, &old_bit); - EXPECT_FALSE(old_bit); - } - auto s = bitmap_->Del(key_); -} - -TEST_P(RedisBitmapTest, BitPosSetBit_BIT) { - uint32_t offsets[] = {0, 123, 1024 * 8, 1024 * 8 + 16, 3 * 1024 * 8, 3 * 1024 * 8 + 16}; - for (const auto &offset : offsets) { - bool bit = false; - bitmap_->SetBit(key_, offset, true, &bit); - } - int64_t pos = 0; - int start_indexes[] = {0, 1 * 8, 124 * 8, 1025 * 8, 1027 * 8, (3 * 1024 + 1) * 8}; - for (size_t i = 0; i < sizeof(start_indexes) / sizeof(start_indexes[0]); i++) { - bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos, /*bit_index=*/true); - EXPECT_EQ(pos, offsets[i]); - } - auto s = bitmap_->Del(key_); -} - -TEST_P(RedisBitmapTest, BitPosNegative_BIT) { - { - bool bit = false; - bitmap_->SetBit(key_, 8 * 1024 - 1, true, &bit); - EXPECT_FALSE(bit); - } - int64_t pos = 0; - // First bit is negative - bitmap_->BitPos(key_, false, 0, -1, true, &pos, /*bit_index=*/true); - EXPECT_EQ(0, pos); - // 8 * 1024 - 1 bit is positive - bitmap_->BitPos(key_, true, 0, -1, true, &pos, /*bit_index=*/true); - EXPECT_EQ(8 * 1024 - 1, pos); - // For bit indexing this should evalue to -1 instead of 8 * 1023 (in the case of byte index) - // This matches the behaviour on redis side - bitmap_->BitPos(key_, false, -1, -1, true, &pos, /*bit_index=*/true); - EXPECT_EQ(-1, pos); - // Last Bit in 1023 byte is positive - bitmap_->BitPos(key_, true, -1, -1, true, &pos, /*bit_index=*/true); - EXPECT_EQ(8 * 1024 - 1, pos); - // Large negative number will be normalized. - bitmap_->BitPos(key_, false, -10000, -10000, true, &pos, /*bit_index=*/true); - EXPECT_EQ(0, pos); - - auto s = bitmap_->Del(key_); -} diff --git a/tests/gocase/unit/type/bitmap/bitmap_test.go b/tests/gocase/unit/type/bitmap/bitmap_test.go index 0b6e8fd597f..5c9024b7762 100644 --- a/tests/gocase/unit/type/bitmap/bitmap_test.go +++ b/tests/gocase/unit/type/bitmap/bitmap_test.go @@ -399,4 +399,173 @@ func TestBitmap(t *testing.T) { require.NoError(t, cmd.Err()) require.EqualValues(t, 2, cmd.Val()) }) + + /* Test cases adapted from redis test cases : https://github.com/redis/redis/blob/unstable/tests/unit/bitops.tcl + */ + t.Run("BITPOS bit=0 with empty key returns 0", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "str").Err()) + cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 0, cmd.Val()) + }) + + t.Run("BITPOS bit=0 with string less than 1 word works", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\xff\xf0\x00", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 12, cmd.Val()) + }) + + t.Run("BITPOS bit=1 with string less than 1 word works", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\x00\x0f\x00", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 1, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 12, cmd.Val()) + }) + + t.Run("BITPOS bit=0 starting at unaligned address", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\xff\xf0\x00", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 0, 1, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 12, cmd.Val()) + }) + + t.Run("BITPOS bit=1 starting at unaligned address", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\x00\x0f\xff", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 1, 1, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 12, cmd.Val()) + }) + + t.Run("BITPOS bit=0 unaligned+full word+reminder", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\xff\xff\xff", 0).Err()) + require.NoError(t, rdb.Append(ctx, "str", "\xff\xff\xff\xff\xff\xff\xff\xff").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\xff\xff\xff\xff\xff\xff\xff\xff").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\xff\xff\xff\xff\xff\xff\xff\xff").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\x0f").Err()) + // Test values 1, 9, 17, 25, 33, 41, 49, 57, 65 + for i := 0; i < 9; i++ { + if i == 6 { + cmd := rdb.BitPosSpan(ctx, "str", 0, 41, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 216, cmd.Val()) + } else { + cmd := rdb.BitPosSpan(ctx, "str", 0, int64(i*8)+1, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 216, cmd.Val()) + } + } + }) + + t.Run("BITPOS bit=1 unaligned+full word+reminder", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\x00\x00\x00", 0).Err()) + require.NoError(t, rdb.Append(ctx, "str", "\x00\x00\x00\x00\x00\x00\x00\x00").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\x00\x00\x00\x00\x00\x00\x00\x00").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\x00\x00\x00\x00\x00\x00\x00\x00").Err()) + require.NoError(t, rdb.Append(ctx, "str", "\xf0").Err()) + // Test values 1, 9, 17, 25, 33, 41, 49, 57, 65 + for i := 0; i < 9; i++ { + if i == 6 { + cmd := rdb.BitPosSpan(ctx, "str", 1, 41, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 216, cmd.Val()) + } else { + cmd := rdb.BitPosSpan(ctx, "str", 1, int64(i*8)+1, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 216, cmd.Val()) + } + } + }) + + t.Run("BITPOS bit=1 returns -1 if string is all 0 bits", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "", 0).Err()) + for i := 0; i < 20; i++ { + cmd := rdb.BitPosSpan(ctx, "str", 1, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, -1, cmd.Val()) + require.NoError(t, rdb.Append(ctx, "str", "\x00").Err()) + } + }) + + t.Run("BITPOS bit=0 works with intervals", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\x00\xff\x00", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 0, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 0, 8, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 16, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 0, 16, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 16, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 0, 16, 200, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 16, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 0, 8, 8, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, -1, cmd.Val()) + }) + + t.Run("BITPOS bit=1 works with intervals", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\x00\xff\x00", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 1, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 8, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 1, 8, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 8, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 1, 16, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, -1, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 1, 16, 200, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, -1, cmd.Val()) + cmd = rdb.BitPosSpan(ctx, "str", 1, 8, 8, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, 8, cmd.Val()) + }) + + t.Run("BITPOS bit=0 changes behavior if end is given", func(t *testing.T) { + require.NoError(t, rdb.Set(ctx, "str", "\xff\xff\xff", 0).Err()) + cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, -1, cmd.Val()) + }) + + t.Run("BITPOS bit=1 fuzzy testing using SETBIT", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "str").Err()) + var max int64 = 524288 + var firstOnePos int64 = -1 + for j := 0; j < 1000; j++ { + cmd := rdb.BitPosSpan(ctx, "str", 1, 0, -1, "bit") + require.NoError(t, cmd.Err()) + require.EqualValues(t, firstOnePos, cmd.Val()) + pos := util.RandomInt(max) + require.NoError(t, rdb.SetBit(ctx, "str", int64(pos), 1).Err()) + if firstOnePos == -1 || firstOnePos > pos { + firstOnePos = pos + } + } + }) + + t.Run("BITPOS bit=0 fuzzy testing using SETBIT", func(t *testing.T) { + var max int64 = 524288 + var firstZeroPos int64 = max + require.NoError(t, rdb.Set(ctx, "str", strings.Repeat("\xff", int(max/8)), 0).Err()) + for j := 0; j < 1000; j++ { + cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") + require.NoError(t, cmd.Err()) + if firstZeroPos == max { + require.EqualValues(t, -1, cmd.Val()) + } else { + require.EqualValues(t, firstZeroPos, cmd.Val()) + } + pos := util.RandomInt(max) + require.NoError(t, rdb.SetBit(ctx, "str", int64(pos), 0).Err()) + if firstZeroPos > pos { + firstZeroPos = pos + } + } + }) + } From a93d2af631a7ff4d5fe5e44b548f6b7a72a53d07 Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Sun, 17 Mar 2024 21:42:51 +0530 Subject: [PATCH 6/8] Fix golangci-lint Signed-off-by: Mohammad Shehar Yaar Tausif --- tests/gocase/unit/type/bitmap/bitmap_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gocase/unit/type/bitmap/bitmap_test.go b/tests/gocase/unit/type/bitmap/bitmap_test.go index 5c9024b7762..55528e30991 100644 --- a/tests/gocase/unit/type/bitmap/bitmap_test.go +++ b/tests/gocase/unit/type/bitmap/bitmap_test.go @@ -550,7 +550,7 @@ func TestBitmap(t *testing.T) { t.Run("BITPOS bit=0 fuzzy testing using SETBIT", func(t *testing.T) { var max int64 = 524288 - var firstZeroPos int64 = max + firstZeroPos := max require.NoError(t, rdb.Set(ctx, "str", strings.Repeat("\xff", int(max/8)), 0).Err()) for j := 0; j < 1000; j++ { cmd := rdb.BitPosSpan(ctx, "str", 0, 0, -1, "bit") From 039e5b9e49536ed7ae13fc859bbe15785a8f325c Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Mon, 18 Mar 2024 14:44:59 +0530 Subject: [PATCH 7/8] Improve lamda function Signed-off-by: Mohammad Shehar Yaar Tausif --- src/types/redis_bitmap.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index d682f6da3fc..dfe17b99d57 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -349,14 +349,6 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i return -1; }; - auto range_in_byte = [](uint32_t byte_start, uint32_t byte_end, uint32_t curr_byte, uint32_t start_bit, - uint32_t end_bit) -> std::pair { - if (curr_byte == byte_start && curr_byte == byte_end) return {start_bit, end_bit}; - if (curr_byte == byte_start) return {start_bit, 7}; - if (curr_byte == byte_end) return {0, end_bit}; - return {0, 7}; - }; - LatestSnapShot ss(storage_); rocksdb::ReadOptions read_options; read_options.snapshot = ss.GetSnapShot(); @@ -371,6 +363,16 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i start_bit_pos_in_byte = u_start % 8; stop_bit_pos_in_byte = u_stop % 8; } + + auto range_in_byte = [start_bit_pos_in_byte, stop_bit_pos_in_byte]( + uint32_t byte_start, uint32_t byte_end, + uint32_t curr_byte) -> std::pair { + if (curr_byte == byte_start && curr_byte == byte_end) return {start_bit_pos_in_byte, stop_bit_pos_in_byte}; + if (curr_byte == byte_start) return {start_bit_pos_in_byte, 7}; + if (curr_byte == byte_end) return {0, stop_bit_pos_in_byte}; + return {0, 7}; + }; + // Don't use multi get to prevent large range query, and take too much memory // Searching bits in segments [start_index, stop_index]. for (uint32_t i = start_segment_index; i <= stop_segment_index; i++) { @@ -411,8 +413,7 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i int bit_pos_in_byte_value = -1; if (is_bit_index) { uint32_t start_bit = 0, stop_bit = 7; - std::tie(start_bit, stop_bit) = range_in_byte(byte_with_bit_start, byte_with_bit_stop, byte_pos_in_segment, - start_bit_pos_in_byte, stop_bit_pos_in_byte); + std::tie(start_bit, stop_bit) = range_in_byte(byte_with_bit_start, byte_with_bit_stop, byte_pos_in_segment); bit_pos_in_byte_value = bit_pos_in_byte_startstop(pin_value[byte_pos_in_segment], bit, start_bit, stop_bit); } else { bit_pos_in_byte_value = bit_pos_in_byte(pin_value[byte_pos_in_segment], bit); From 1f036d6260fb623bcbae3e3703ce01d7ce84a05b Mon Sep 17 00:00:00 2001 From: Mohammad Shehar Yaar Tausif Date: Mon, 18 Mar 2024 23:07:36 +0530 Subject: [PATCH 8/8] Fix review suggestions Signed-off-by: Mohammad Shehar Yaar Tausif --- src/common/bit_util.h | 4 ---- src/types/redis_bitmap.cc | 6 ++++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/common/bit_util.h b/src/common/bit_util.h index 898e3cdd7fb..9c23d78bb8d 100644 --- a/src/common/bit_util.h +++ b/src/common/bit_util.h @@ -103,10 +103,6 @@ static inline void SetBitTo(uint8_t *bits, int64_t i, bool bit_is_set) { * not a single set bit in the bitmap. In this special case -1 is returned. * */ inline int64_t RawBitpos(const uint8_t *c, int64_t count, bool bit) { - if (count == 0) { - return -1; - } - int64_t res = 0; if (bit) { diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index dfe17b99d57..49151630127 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -306,6 +306,8 @@ rocksdb::Status Bitmap::BitCount(const Slice &user_key, int64_t start, int64_t s rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, int64_t stop, bool stop_given, int64_t *pos, bool is_bit_index) { + if (is_bit_index) DCHECK(stop_given); + std::string raw_value; std::string ns_key = AppendNamespacePrefix(user_key); @@ -326,8 +328,8 @@ rocksdb::Status Bitmap::BitPos(const Slice &user_key, bool bit, int64_t start, i auto size = static_cast(metadata.size) * static_cast(to_bit_factor); std::tie(start, stop) = BitmapString::NormalizeRange(start, stop, size); - auto u_start = static_cast(start); - auto u_stop = static_cast(stop); + auto u_start = static_cast(start); + auto u_stop = static_cast(stop); if (u_start > u_stop) { *pos = -1; return rocksdb::Status::OK();