search

DiceDB / dice

DiceDB is a redis-compliant, reactive, scalable, highly-available, unified cache optimized for modern hardware.
https://dicedb.io/
Other
6.83k stars 1.08k forks source link

fix JSONPath inconsistencies #1266

Open N3XT14 opened 1 week ago

N3XT14 commented 1 week ago

@JyotinderSingh.

Sorry for the delay. I was busy with something urgent at work and couldn't give enough time.

Have a look at this PR. It's not completed yet but good portion of it is completed. Unfortunately, I am unable to find how to enable the resp3 protocol.

Also, in the Data Link shared the multi path examples seems incorrect in the array example.

Currently, I am testing it on my end but since certain parts such as RESP3 and custom formats are pending I am not asking for a merge yet.

I wanted to also open up a discussion on certain outputs that Redis returns such as for example "accessing negative index on out of bounds for an arrray example", etc.

Should we be following that as I believe we should instead return error for such cases instead of nil or the way redis handles.

Fixes #1002

JyotinderSingh commented 1 week ago

We're not looking to support RESP3 at this stage. Let's stick with RESP2.

Is RESP3 support a blocker in any way?

N3XT14 commented 1 week ago

Nope, definitely not a blocker.

JyotinderSingh commented 1 week ago

Nope, definitely not a blocker.

In that case we can remove the related code and limit this PR to just the jsonpath changes.

JyotinderSingh commented 5 days ago

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

N3XT14 commented 2 days ago

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

Hi Yes,

Apologies, I was traveling the entire week so couldn't push.

I have pushed the latest changes. Test cases required changes in output structure mainly. I ran linter on my local before pushing it through some warnings and error at other parts of the code. I am not sure why it didn't caught attention earlier. Currently, GitHub actions is seeking approval

JyotinderSingh commented 1 day ago

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

Hi Yes,

Apologies, I was traveling the entire week so couldn't push.

I have pushed the latest changes. Test cases required changes in output structure mainly. I ran linter on my local before pushing it through some warnings and error at other parts of the code. I am not sure why it didn't caught attention earlier. Currently, GitHub actions is seeking approval

Thanks for the update! Since you've never pushed a commit into the repository before your PRs require a manual github actions run. I've started the CI for this change.

JyotinderSingh commented 1 day ago

Looks like there are some test failure, could you please take a look.

N3XT14 commented 1 day ago

Looks like there are some test failure, could you please take a look.

There are two test cases failing:

  1. JSON.MGET in resp: This issue likely stems from a mismatch in the output interface, which occurred during migration. Since my code doesn't directly impact the MGET test cases, I believe the failure is related to the migration process rather than any recent changes in the code.

  2. JSON.GET for UnsupportedJSONPathOperations (including regex): Previously, regex was not supported in the JSONPath operations, but now it is. I have updated the test cases to reflect this change. However, another issue arises from the parsing logic in the ParseCommand function, which is located in the parsecommand.go file. The function currently only considers double quotes to determine if subsequent characters belong to the same path. In earlier test cases, single quotes were used, causing the parser to incorrectly treat them as separate paths. I have updated the test cases to use double quotes, but we need to improve the handling of quotes in the ParseCommand function to avoid similar issues in the future.