search

DiceDB / dice

DiceDB is an in-memory real-time database with SQL-based reactivity. It is hyper-optimized for building and scaling truly real-time applications on modern hardware while being a drop-in replacement for Redis.
https://dicedb.io/
Other
3.96k stars 492 forks source link

Report inconsistency in the command `JSON.SET` #381

Open arpitbbhayani opened 3 weeks ago

arpitbbhayani commented 3 weeks ago

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

Go through the official documentation of the command JSON.SET 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

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

  1. use the template and provide the following details
  2. steps to reproduce (series of commands)
  3. observed output on DiceDB
  4. 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.

kushal0511-not commented 3 weeks ago

@arpitbbhayani can i take this up?

arpitbbhayani commented 3 weeks ago

@kushal0511-not go for it :)

kushal0511-not commented 2 weeks ago

We have some inconsistency in command due to how we treat JSONPath in JSON.SET command. here are few examples. 1 ) dice : Screenshot from 2024-08-28 17-51-32 Redis 7.2.5 : Screenshot from 2024-08-28 17-51-58

2 ) dice : Screenshot from 2024-08-28 17-54-26 Redis 7.2.5 : Screenshot from 2024-08-28 17-54-10

I will raise PR to fix it

cc @JyotinderSingh

kushal0511-not commented 1 week ago

I have tried other libraries for jsonpath, but the current library which we are using is the closest to redis(7.2.5) jsonpath. As we have inconsistency with the redis (like this library parse some json paths differently than redis 7.2.5) , what should be the next steps for this issue. This will affect almost all JSON related commands. Should we fork this library and make changes to it OR Should we create our own from scratch.

@JyotinderSingh @soumya-codes

JyotinderSingh commented 4 days ago

I have tried other libraries for jsonpath, but the current library which we are using is the closest to redis(7.2.5) jsonpath. As we have inconsistency with the redis (like this library parse some json paths differently than redis 7.2.5) , what should be the next steps for this issue. This will affect almost all JSON related commands. Should we fork this library and make changes to it OR Should we create our own from scratch.

@JyotinderSingh @soumya-codes

Thanks for looking into this @kushal0511-not, let's keep this issue open for now, we can look into fixing it at a later stage. The partial support for the syntax is enough for the existing use cases.

apoorvyadav1111 commented 2 days ago

Hi, I have found another inconsistency for JSON.SET when we try to create a string at the root path. dice: image redis: image

I am working on JSON.STRLEN and found this deviation. Since this is an open issue, I feel reporting it here is better than raising another issue. Regards, Apoorv