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

#332: fixes inconsistency in the incr command #677

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

Maveric-k07
Copy link
Contributor

@Maveric-k07 Maveric-k07 commented Sep 21, 2024

fixes #332

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.
edit: saw that there is already progress on SETEX command - will be a good addition.

cc: @JyotinderSingh Apologies for the delay in creating the PR;

@JyotinderSingh JyotinderSingh changed the title fix inconsistency in the incr command #332: fixes inconsistency in the incr command Sep 23, 2024
Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these fixes @Maveric-k07! The changes look good. Approved

@JyotinderSingh JyotinderSingh merged commit 0cca565 into DiceDB:master Sep 23, 2024
2 checks passed
shardul08 pushed a commit to shardul08/dice that referenced this pull request Sep 23, 2024
Co-authored-by: Jyotinder Singh <jyotindrsingh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report inconsistency in the command INCR
2 participants