Report inconsistency in the command INCR

[arpitbbhayani]

This issue is all about ensuring we are as close to Redis as possible. The command in focus for this issue is INCR.

Go through the official documentation of the command INCR on Redis and identify the inconsistencies. The inconsistencies could be in

1. unhandled edge case
2. unexpected behavior
3. unsupported option

Because we are trying to be compatible with Redis v7.2.5, I would recommend you try out different variants of the command with different inputs on that specific version. The instructions on running Redis v7.2.5 locally

• from source code
• use Docker

Once you find the discrepancy, you can either

1. raise an issue on Dice repository with details, or
2. try to fix it yourself and raise a PR

If you are raising the issue, make sure you provide the details such as

0. use the template and provide the following details
1. steps to reproduce (series of commands)
2. observed output on DiceDB
3. observed output on Redis v7.2.5

Also, feel free to update the documentation and raise the PR in the docs repository.

You will need to go deeper into the command make sure you are covering all cases and reporting the inconsistencies or fixing them. The deeper the work, the better our stability will be. Also, it is possible that we do not find any discrepancies, so please mention the same in the comment on this issue. Mention the PR or issue links that you create under this issue.

[Maveric-k07]

can i take this up please

[vanshavenger]

Hey @arpitbbhayani

I've run a series of tests to check for inconsistencies and edge cases that weren't originally covered in TESTincr. Here's what I found:

1. Incrementing a Non-Existent Key:

Expected Behavior: The key should be created and set to 1.
Result: Worked as expected.

2. Incrementing a String Representing an Integer:

Expected Behavior: The value should increment correctly.
Result: Worked as expected.

3. Incrementing a Non-Integer String:

Expected Behavior: An error should be returned.
Result: Worked as expected.

4. Incrementing a Float String:

Expected Behavior: An error should be returned.
Result: Worked as expected.

5. Incrementing from the Minimum 64-Bit Integer:

Expected Behavior: The value should increment correctly without issues.
Result: Worked as expected.

6. Incrementing a Key with Expiration:

Expected Behavior: The key should expire as expected during the increment operation, resetting its value to 0 upon expiration.
Result: Worked as expected.

7. Incrementing to the Maximum 64-Bit Integer:

Expected Behavior: The value should increment to math.MaxInt64 without overflow. Further increments should return an "Out of bounds" error.
Result: Worked as expected.

I tested how the system handles unsupported options by providing empty spaces, tab spaces, and multiple options. It successfully identified and rejected unsupported options.

I’m comfortable with the code and test structure, i found some of tests can be added to the TESTIncr, should i raise a PR for it, I am beginner in golang but can understand the codebase. : @arpitbbhayani

Also additionally, I was testing it with JSON.SET var and different structures, It returns PONG for them and when i pass it some arguements it return incorrect Args for "ping" command. So when these commands get implemented with can test it here too for edge cases by doing INCR on these structures.

[Maveric-k07]

Inconsistencies found only with the error messages and not in functionality.

1. Data types:
   Redis: "INCR strkey": "Error: value is not an integer or out of range"
   DiceDB: "INCR strkey": "Error: the operation is not permitted on this encoding"

   Redis: "INCR floatkey": "Error: value is not an integer or out of range"
   DiceDB: "INCR floatkey": "Error: the operation is not permitted on this encoding"

   This is the case for all non-integer values.

   The error messages are different, though the behavior (not incrementing non-integer values) is consistent.

2. Overflow handling:
   Redis: "INCR maxint": "Error: increment or decrement would overflow"
   DiceDB: "INCR maxint": "Error: value is out of range"

   The error messages differ, but both prevent overflow.

Cases working correctly:

1. Basic functionality (INCR and GET)
2. Incrementing large integers within range, integer string value
3. Negative numbers handling
4. Zero handling
5. Large increments
6. Key naming (empty string and colon-separated keys)
7. Multiple increments
8. INCR and DEL interaction
9. INCR and RENAME
10. INCR and COPY

To test with keys that have expiry I had to run the following:

"INCR with EXPIRE": [
        "SET expirablekey 10",
        "EXPIRE expirablekey 60",
        "INCR expirablekey",
        "TTL expirablekey"
]

This can be achieved by using SETEX. Could be a good addition.
Another good to have will be the FLUSHALL command.
Can raise a PR making the error messages consistent. Let me know if i should go ahead with it. @arpitbbhayani
as i see @vanshavenger has some thoughts as well.

[vanshavenger]

I completely agree with @Maveric-k07, error messages are different and can be made consistent,
and one other point SETEX is deprecated, but SET key1 10 EX 5, you can use, it i also in dice DB, i tested it last night too.

And yes i agree that FLUSHALL command will be good to have.

@Maveric-k07 can work on making error messages consistent
and i can add the additional test cases to TestINCR.

[Maveric-k07]

Having multiple PRs for this might not be a good idea. Can you take up the change in error messages as well? @vanshavenger or I can push my commits to your pr once you're done

[vanshavenger]

I guess i make some commits and then you can push changes to my PR or vice-versa
as i have locally implemented those tests and have to check for error messages,
just waiting for @arpitbbhayani suggestions on the same.

[AshwinKul28]

@Maveric-k07 @vanshavenger Thanks for all the work. Are you guys still waiting for Arpit's response to this?
Let's discuss this in the discord discussions and we can close this out asap. Thanks.

[Maveric-k07]

@vanshavenger can you take up the changes in the error messages as well please, it's a small change in addition to the test cases you wanted to add. feel free to ping in the the discord channel.

[vanshavenger]

@Maveric-k07 I may not get the time before the weekends because of the job, will raise a PR for Tests suite by night as they are already handy with me, last time i made

[AshwinKul28]

Hey @vanshavenger I Hope you are doing well. Can you please update any latest findings or work you have done?

[arpitbbhayani]

Hello @Maveric-k07,

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.

[Maveric-k07]

I will make the necessary changes, I was waiting on vansh for an update but I'll create a PR today.

[arpitbbhayani]

Hello @Maveric-k07,

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.