You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/12/28 14:02:52 UTC
[GitHub] [incubator-kvrocks] lt1946 opened a new issue, #1212: new bug for command: bitpos
lt1946 opened a new issue, #1212:
URL: https://github.com/apache/incubator-kvrocks/issues/1212
### Search before asking
- [X] I had searched in the [issues](https://github.com/apache/incubator-kvrocks/issues) and found no similar issues.
### Version
latest
### Minimal reproduce step
from redis command : [https://redis.io/commands/bitpos/](url)
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!
--
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: issues-unsubscribe@kvrocks.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on issue #1212: new bug for command: bitpos
Posted by GitBox <gi...@apache.org>.
tisonkun commented on issue #1212:
URL: https://github.com/apache/incubator-kvrocks/issues/1212#issuecomment-1370581586
I'd suggest adding a comment/description in the document https://kvrocks.apache.org/docs/supported-commands#bit-commands and closing this issue.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-2002326558
> ( 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
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on issue #1212: new bug for command: bitpos
Posted by GitBox <gi...@apache.org>.
torwig commented on issue #1212:
URL: https://github.com/apache/incubator-kvrocks/issues/1212#issuecomment-1366797037
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.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-2002189022
@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.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on issue #1212: new bug for command: bitpos
Posted by GitBox <gi...@apache.org>.
torwig commented on issue #1212:
URL: https://github.com/apache/incubator-kvrocks/issues/1212#issuecomment-1366801588
@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
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITOPS doesn't support BIT option [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1988262449
@sheharyaar Assign! You can refer to https://github.com/apache/kvrocks/pull/2087
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1991019104
Ok, I will move on with support for bit in `RawBitPos` and I will accordingly overload the functions to keep them backwards compatible.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-2002190695
Ok, I will add a fix soon.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITOPS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1988225181
@jihuayu , I would like to take up this issue, please assign me this.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-2002199455
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)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-2000449365
@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.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1998408811
@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 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) ?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU closed issue #1212: BITPOS doesn't support BIT option
URL: https://github.com/apache/kvrocks/issues/1212
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1998695669
@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](https://github.com/apache/kvrocks/blob/815d11f205105f845b3718e7081bca703d163cf3/tests/gocase/unit/type/bitmap/bitmap_test.go)" can cover all cases, you don't need to add C++ tests. We prioritize using go test.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITOPS doesn't support BIT option [kvrocks]
Posted by "sheharyaar (via GitHub)" <gi...@apache.org>.
sheharyaar commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1988952298
@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 :
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).
So which should I go with ?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITOPS doesn't support BIT option [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1989690338
@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.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] BITPOS doesn't support BIT option [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on issue #1212:
URL: https://github.com/apache/kvrocks/issues/1212#issuecomment-1989950905
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
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org