dice icon indicating copy to clipboard operation
dice copied to clipboard

Inconsistent `BITPOS`: BITPOS bit=0 works with intervals

Open lucifercr07 opened this issue 1 year ago • 10 comments

Steps to reproduce

Run the commands mentioned in the test on Line 365 in the file https://github.com/AshwinKul28/dice-tests/blob/main/tcltests/unit/bitpos.tcl#L365.

Expected output

The expected output when the above set of commands when run on Redis

Expected [r bitpos str 0 8 -1 bit]

Observed output

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

16 (context: type eval line 10 cmd {assert {[r bitpos str 0 8 -1 bit]

The steps to run the test cases are mentioned in the README of the dice-tests repository.

Expectations for resolution

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

  1. Changes in the dice repository code to meet the expected behavior.
  2. Successful run of the tcl test behavior.

You can find the tests under the 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

lucifercr07 avatar Sep 20 '24 18:09 lucifercr07

Hi @lucifercr07 , can you assign this task to me?

Nijin-P-S avatar Sep 21 '24 03:09 Nijin-P-S

Hi @lucifercr07 Can I take this up ? Would love to work on it. Thanks

jainbhavya53 avatar Sep 21 '24 09:09 jainbhavya53

@jainbhavya53 Go for it.

@Nijin-P-S Assigned another issue to you!

Thanks guys for showing interest.

AshwinKul28 avatar Sep 21 '24 10:09 AshwinKul28

Hi @AshwinKul28 @lucifercr07 I executed the mentioned test case in dice and redis, found that assert statements and dice output are incorrect in some scenarios:

Test Case Expected Output Dice Output Remarks
assert {[r bitpos str 0 0 -1] == 0} 0 0
assert {[r bitpos str 0 1 -1] == 16} 8 8 Assert Statement incorrect
assert {[r bitpos str 0 2 -1] == 16} 16 16
assert {[r bitpos str 0 2 200] == 16} 16 16
assert {[r bitpos str 0 1 1] == -1} 8 8 Assert Statement incorrect
assert {[r bitpos str 0 0 -1 bit] == 0} 0 0
assert {[r bitpos str 0 8 -1 bit] == 16} 8 64 Assert Statement and Dice Output Incorrect
assert {[r bitpos str 0 16 -1 bit] == 16} 16 -1 Dice Output Incorrect
assert {[r bitpos str 0 16 200 bit] == 16} 16 -1 Dice Output Incorrect
assert {[r bitpos str 0 8 8 bit] == -1} 8 64 Assert Statement and Dice Output Incorrect

Couple of AIs:

  • BITPOS command is broken when we specify interval query for BIT
  • assert statements need to fixed
  • Issue https://github.com/DiceDB/dice/issues/670 is duplicate of current one. Whatever fix is done for given scenario will apply to it as well. To avoid duplicate effort, please close #670 by marking it as duplicate.

Thanks

jainbhavya53 avatar Sep 21 '24 18:09 jainbhavya53

Same goes for https://github.com/DiceDB/dice/issues/667 and https://github.com/DiceDB/dice/issues/668

jainbhavya53 avatar Sep 21 '24 19:09 jainbhavya53

For #670 , ran the scenarios following are the results:

Test Case Expected Output Dice Output Remarks
assert {[r bitpos str 1 0 -1] == 8} 1 1
assert {[r bitpos str 1 1 -1] == 8} 9 9
assert {[r bitpos str 1 2 -1] == -1} 18 18
assert {[r bitpos str 1 2 200] == -1} 18 18
assert {[r bitpos str 1 1 1] == 8} 9 9
assert {[r bitpos str 1 0 -1 bit] == 8} 1 1
assert {[r bitpos str 1 8 -1 bit] == 8} 9 65 Dice Output Wrong
assert {[r bitpos str 1 16 -1 bit] == -1} 18 -1 Dice Output Wrong
assert {[r bitpos str 1 16 200 bit] == -1} 18 -1 Dice Output Wrong
assert {[r bitpos str 1 8 8 bit] == 8} -1 65 Dice Output Wrong

Observations:

  • Same as mentioned in above comment.
  • All assert statements are incorrect

jainbhavya53 avatar Sep 21 '24 19:09 jainbhavya53

@dankot12 Please refer to above comment

jainbhavya53 avatar Sep 25 '24 03:09 jainbhavya53

We haven't implemented APPEND function yet, that's why below tests are failing. https://github.com/DiceDB/dice/issues/667 and https://github.com/DiceDB/dice/issues/668

jainbhavya53 avatar Sep 25 '24 09:09 jainbhavya53

PR: https://github.com/DiceDB/dice/pull/716

jainbhavya53 avatar Sep 25 '24 09:09 jainbhavya53

Hi @jainbhavya53 , I agree with your point, I am working on #668 and #667 to implement APPEND

dankot12 avatar Sep 26 '24 18:09 dankot12