Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent SETBIT: integer value corresponding to a key cannot be updated using SETBIT in certain situations #1115

Open
Yashasv-Prajapati opened this issue Oct 16, 2024 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@Yashasv-Prajapati
Copy link

Steps to reproduce

  1. Start the dicedb server and connect to it through whatever client(redis-cli or something else) you prefer.
  2. Run the following commands in sequence.
    a. SET <key> 10
    b. SETBIT <key> 1 1
    c. SETBIT <key> 3 0

Expected output

The expected output when the above set of commands (maybe when run on Redis) should be the previously set bit on that offset.

For A -> (integer) 0 -> Previous bit at this position
For B -> (integer) 1 -> Previous bit at this position

Observed output

The observed output when the above set of commands when run on DiceDB

For A -> (error) WRONGTYPE Operation against a key holding the wrong kind of value
For B -> (error) WRONGTYPE Operation against a key holding the wrong kind of value

Also, if you try to set the same bit as the previous bit at that offset, it seems to be working fine without any error. Like the following command.

SETBIT <key> 1 0 -> Command A
SETBIT <key> 3 1 -> Command B

Expectations for resolution

This issue will be considered resolved when the following things are done

  1. Changes in the dice code to meet the expected behavior
  2. Addition of relevant test case to ensure we catch the regression

You can find the tests under the integration_tests directory of the dice repository and the steps to run are in the README file. Refer to the following links to set up DiceDB and Redis 7.2.5 locally

Follow up

Once this is completed, drop a comment in issue #813 regarding the same.

@kaushal-003
Copy link

@apoorvyadav1111 Can I work on this issue?

@apoorvyadav1111
Copy link
Contributor

Hi @Yashasv-Prajapati , Would you be taking this up? If not, Shall I go ahead and assign @kaushal-003 to this issue?

@apoorvyadav1111 apoorvyadav1111 added the bug Something isn't working label Oct 16, 2024
@Yashasv-Prajapati
Copy link
Author

Hi @Yashasv-Prajapati , Would you be taking this up? If not, Shall I go ahead and assign @kaushal-003 to this issue?

No thanks, please assign it to @kaushal-003 😊

@apoorvyadav1111
Copy link
Contributor

Hey @kaushal-003 , assigned.

@kaushal-003
Copy link

In evalSETBIT function integers are considered as byte arrays. For example, when we run the command SET <key> 10, the resulting byte array is:

[49 48]

Here, 49 is the ASCII value for '1', and 48 is the ASCII value for '0'.

When we execute the command SETBIT <key> 1 1, the bit offset is applied starting from the most significant bit (MSB). This changes the byte array to:

[113 48]

This happens because 113 is the result of setting a bit in 49 (i.e., 49 + 64 = 113). Since each byte represents 8 bits, manipulating these bits directly changes the stored value.

The new byte array now contains 113, which is outside the ASCII range for the integer characters, causing an error when trying to interpret the data as an integer. If the bit is not changed, the value remains within the valid range for integers, which is why no errors occur in those cases.

@kaushal-003
Copy link

@Yashasv-Prajapati @apoorvyadav1111, What is the expected behaviour for evalSETBIT?

@apoorvyadav1111
Copy link
Contributor

Hi @kaushal-003 , I can dig into this to share the expected behaviour for evalSETBIT. Meanwhile, I would suggest you to try running the same commands in redis and see if that is something acceptable and try to match the results to Redis output.
Please note that this issue will not be closed until its parent issue #1017 is closed and the PR for this issue is rebased.

Thanks

@kaushal-003
Copy link

I tried running it on redis and I got following result

127.0.0.1:6379> set key 10
OK
127.0.0.1:6379> setbit key 1 1
(integer) 0
127.0.0.1:6379> get key
"q0"
127.0.0.1:6379> 

113 in ascii is q.
key is not integer now, but it is not throwing any error also.

@arpitbbhayani
Copy link
Contributor

Hello @kaushal-003,

There has been no activity on this issue for the past 5 days.
It would be awesome if you keep posting updates to this issue so that we know you are actively working on it.

We are really eager to close this issue at the earliest, hence if we continue to see the inactivity, we will have to reassign the issue to someone else. We are doing this to ensure that the project maintains its momentum and others are not blocked on this work.

Just drop a comment with the current status of the work or share any issues you are facing. We can always chip in to help you out.

Thanks again.

@kaushal-003
Copy link

I have linked the PR resolving the issue; it can only be merged after #1017 is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants