BITOPS doesn't support BIT option

[lt1946]

trafficstars

Search before asking

• [X] I had searched in the issues and found no similar issues.

Version

latest

Minimal reproduce step

from redis command : https://redis.io/commands/bitpos/ redis> SET mykey "\x00\xff\xf0" "OK"

What did you expect to see?

redis> BITPOS mykey 1 7 15 BIT (integer) 9

What did you see instead?

redis> BITPOS mykey 1 7 15 BIT (integer) 57

Anything Else?

No response

Are you willing to submit a PR?

• [ ] I'm willing to submit a PR!

[torwig]

Hi @lt1946, thank you for your report. On my machine, with Redis I have the following:

[yaroslav@localhost ~]$ redis-cli
127.0.0.1:6379> SET mykey1 "\x00\xff\xf0"
OK
127.0.0.1:6379> BITPOS mykey1 1 7 15 BIT
(integer) 8

However, you said that you expect to see 9.

Kvrocks behaves in the following way:

127.0.0.1:6666> SET mykey "\x00\xff\xf0"
OK
127.0.0.1:6666> BITPOS mykey 1 7 15 BIT
(integer) -1

And it needs an investigation.

[torwig]

@lt1946 I got the reason why the result is -1. Currently, the BITPOS command doesn't support BIT flag, so it considers start and end values as bytes.

127.0.0.1:6379> BITPOS mykey1 1 1 2
(integer) 8
127.0.0.1:6379> BITPOS mykey1 1 2 3
(integer) 16

[tisonkun]

I'd suggest adding a comment/description in the document https://kvrocks.apache.org/docs/supported-commands#bit-commands and closing this issue.

[sheharyaar]

@jihuayu , I would like to take up this issue, please assign me this.

[jihuayu]

@sheharyaar Assign! You can refer to https://github.com/apache/kvrocks/pull/2087

[sheharyaar]

@jihuayu , I think of two approaches :

1. We can convert the bits to bytes and when the bit position is retrieved we can check if it falls with [start,end) bits.

• This would allow us not to change the semantics of BitmapString::BitPos (https://github.com/apache/kvrocks/blob/unstable/src/types/redis_bitmap_string.cc#L102-L129) which is called by BitMap::BitPos (https://github.com/apache/kvrocks/blob/unstable/src/types/redis_bitmap.cc#L318-L321).

Before that call we can store the original start and stop and then later compare them with the answer, like this : File : redis_bitmap.cc

2. We can change the semantics of BitmapString::BitPos, Bitmap::BitPos and utl::msb::RawBitPos https://github.com/apache/kvrocks/blob/1f3913f7942e370581a6c7ca7f9c99cdb7f58479/src/common/bit_util.h#L105-L145 We can overload BitPos to make it backward compatible too.

So which should I go with ?

[jihuayu]

@sheharyaar Great! Thank you for the detailed plan. I prefer method 2. I want to hear @mapleFU's opinion, he has more experience in this area.

[mapleFU]

Redis itself uses a "bitmask" to mask the implementation: https://github.com/redis/redis/blob/unstable/src/bitops.c#L888

E.g. if bit 1 100, it will change to "byte", and mask the first / last byte with the bitmask. I think it might be ok to do like this. However, you can finish in any way you like if you think that's convient. Personally, I think support "bit" in RawBitpos maybe a better way

[sheharyaar]

Ok, I will move on with support for bit in RawBitPos and I will accordingly overload the functions to keep them backwards compatible.

[sheharyaar]

@jihuayu, I have created a draft PR for this issue, I need to add new test cases. In the tests, the use_bitmap variable is present, if it is false then value type is string, else bitmap. I want to know how can I use bitmap instead of string to test my input in kvrocks cli (not in the tests) ?

[jihuayu]

@sheharyaar Good job! Bitmap commands can both work on string & bitmap type keys. If you use set key bitmap_value to create keys, it is a string type key. If you use setbit key 0 1 to create keys, it is a bitmap type key.

Other, if "go test" can cover all cases, you don't need to add C++ tests. We prioritize using go test.

[sheharyaar]

@jihuayu @mapleFU , the PR is almost complete and I have added the test cases. However there is a small issue. Currently RawBitPos returns count_bytes * 8 (i.e. the number of bits) if the test bit is '0' and no such bits are found in the range. It is so that, in the function BitmapStriing::BitPos we can return -1 if stop_given is true and count_bytes * 8 if stop_given is false. But if we directly add support for bits directly to this function, then in that case it causes a bug.

Example if stored string is "\x00\x00\x00" Query > BITPOS mykey 0 2 3 BIT

This should return 2. But if we calculate total count (stop-start+1) that comes out to be 2 and also the value returned from BitRawPos is 2 (valid).

Then this relation evalutes to true and returns -1 instead of 2. https://github.com/apache/kvrocks/blob/unstable/src/types/redis_bitmap_string.cc#L122-L125

Possible solutions :

1. If we fix this in RawBitPos, it will deviate from redis' implementation. Redis' RawBitPos returns bytes*8, but the bit is handled in the caller function (BitmapString::BitPos) in this case.
2. We handle bits in BitmapString::BitPos and let RawBitPos be the same

This bug doesn't affect Bitmaps as they return -1 even if test bit is 0.

[jihuayu]

@sheharyaar Thanks, those 2 ways both are LGTM.

We only need to be compatible with the Redis protocol; the internal implementation does not need to be identical.

[sheharyaar]

Ok, I will add a fix soon.

[mapleFU]

1. Redis' impl uses code below: https://github.com/redis/redis/blob/unstable/src/bitops.c#L984-L997
2. bitpos util syntax would be clear: returning the "kth" bit in the input.

I think we'd better keep RawBitpos clean and clear, and handling the dirty logic in BitmapString::BitPos.

( besides, I think stop_given and bit is really tricky in redis, sigh)

[sheharyaar]

( besides, I think stop_given and bit is really tricky in redis, sigh)

Yes, I agree too. I will make changes to handle the bit logic in BitPos instead of RawBitPos