dice icon indicating copy to clipboard operation
dice copied to clipboard

Add support for JSON

Open Maveric-k07 opened this issue 1 year ago • 8 comments

#165 Added support for JSON.SET and JSON.GET operations; (jsonpath operations is wip)

@JyotinderSingh the existing Encode function seems to be sophisticated enough for handling the encoding of the json strings to RESP2.

However, in RESP3 standard; things have been changed a bit in case of handling of maps

Kindly take a look at the following docs/discussion: https://redis.io/docs/latest/develop/data-types/json/resp3/ https://github.com/RedisJSON/RedisJSON/issues/1090 https://redis.io/docs/latest/commands/json.set/

Maveric-k07 avatar Jul 28 '24 06:07 Maveric-k07

Thanks for the changes @Maveric-k07.

Please rebase this PR on the latest master to resolve any conflicts. Will review this once that is done.

JyotinderSingh avatar Jul 28 '24 07:07 JyotinderSingh

@Maveric-k07 Regarding RESP3, we only aim to support RESP2 for now.

JyotinderSingh avatar Jul 28 '24 10:07 JyotinderSingh

@JyotinderSingh Alright, let me know if it looks good; I will keep working on #166

Maveric-k07 avatar Jul 28 '24 10:07 Maveric-k07

Thanks for the contribution.

I have some thoughts:

We can probably consider replacing the standard encoding/json library with github.com/valyala/fastjson for both GET and SET operations. fastjson provides efficient parsing and manipulation of JSON objects.

Also, with this library it may be better to store JSON as []byte instead of interface{}. This can help reduce memory usage and will also align better with fastjson's API.

JSONGET

var parser fastjson.Parser

obj := Get(key)
// validations ...    

jsonBytes, ok := obj.Value.([]byte)
if !ok {
    ...
}
v, err := parser.ParseBytes(jsonBytes)

JSONSET

Also, considering the upcoming features we plan to implement, such as JSON Path support (#166) the fastjson library can support that natively (https://pkg.go.dev/github.com/valyala/fastjson#example-Value.Set)

I'm just considering a path separated json path provided to us in the args, but we can implement the official redis standard in the actual PR

var parser fastjson.Parser

key := args[0]
path := args[1:len(args)-1]
jsonValue := args[len(args)-1]

obj := Get(key)
var v *fastjson.Value
if obj != nil {
    // type and encoding validation ...
    jsonBytes, _ := obj.Value.([]byte)
    v, _ = parser.ParseBytes(jsonBytes)
}
    
if v == nil {
    v = fastjson.MustParse("{}")
}

newValue, err := parser.Parse(jsonValue)

if len(path) == 0 {
    v = newValue
} else {
    err = v.Set(path, newValue)
    if err != nil {
        return Encode(errors.New("ERR invalid path"), false)
    }
}

Would like to know your thoughts around this.

@arpitbbhayani what do you think about this?

JyotinderSingh avatar Jul 28 '24 12:07 JyotinderSingh

@JyotinderSingh Thank you for your detailed feedback.

I agree that storing JSON as []byte instead of interface{} is a good approach. It will reduce memory usage and provide better compatibility with fastjson's API. This change aligns well with future implementations.

Adding below a POC for the suggestions using fastjson; all test cases passing in tests/json_test.go passing.

var parser fastjson.Parser


// evalJSONGET retrieves a JSON value stored at the specified key
func evalJSONGET(args []string) []byte {
if len(args) < 1 {
 return Encode(errors.New("ERR wrong number of arguments for 'JSON.GET' command"), false)
}

key := args[0]
obj := Get(key)

if obj == nil || hasExpired(obj) {
 return RESP_NIL
}

objType, _ := ExtractTypeEncoding(obj)
if objType != OBJ_TYPE_JSON {
 return Encode(errors.New("WRONGTYPE Operation against a key holding the wrong kind of value"), false)
}

jsonBytes, ok := obj.Value.([]byte)
if !ok {
 return Encode(errors.New("ERR invalid JSON value"), false)
}

v, err := parser.ParseBytes(jsonBytes)
if err != nil {
 return Encode(errors.New("ERR invalid JSON"), false)
}

return Encode(v.String(), false)
}

// evalJSONSET stores a JSON value at the specified key
func evalJSONSET(args []string) []byte {
 if len(args) < 3 {
  return Encode(errors.New("ERR wrong number of arguments for 'JSON.SET' command"), false)
 }

 key := args[0]
 // path := args[1 : len(args)-1]
 jsonStr := args[len(args)-1]

 v, err := parser.Parse(jsonStr)
 if err != nil {
  return Encode(errors.New("ERR invalid JSON"), false)
 }

 jsonBytes := v.MarshalTo(nil)
 obj := NewObj(jsonBytes, -1, OBJ_TYPE_JSON, OBJ_ENCODING_JSON)
 Put(key, obj)

 return RESP_OK
}

As for the JSON PATH operations, I think we can build on-top of your idea and converge our way to the redis standard in #166

Maveric-k07 avatar Jul 28 '24 16:07 Maveric-k07

@Maveric-k07 @JyotinderSingh I would recommend let's go for a quick implementation for now and store interface{}, once we hit the roadblock then we can always optimize.

My concern is with query time capabilities. I think it would be better if we store JSON as map[string]interface{} the reason being we will make query time more efficient as we need not parse and query the JSON objects. Let me know if my understanding about the approaches has some gaps.

Also, I have updated the tutorial with the changes discussed last week. https://dicedb-docs.netlify.app/tutorials/realtime-leaderboard/

Take a look.

arpitbbhayani avatar Jul 29 '24 18:07 arpitbbhayani

@JyotinderSingh would like to know your thoughts on it please, to finalize which way we are going.

I've been working on implementation of the jsonpath operations using fastjson and had trouble getting it working properly. I stumbled upon another project ajson that is providing us with out-of-the-box support for jsonpath operations, which we can use to ship quickly for the GET, SET and partial update via SET operations. Other JSON commands supported by redis can be added as well. Kindly take a look and let me know if i should go ahead with the implementation with ajson.

Maveric-k07 avatar Jul 30 '24 12:07 Maveric-k07

@JyotinderSingh would like to know your thoughts on it please, to finalize which way we are going.

I've been working on implementation of the jsonpath operations using fastjson and had trouble getting it working properly. I stumbled upon another project ajson that is providing us with out-of-the-box support for jsonpath operations, which we can use to ship quickly for the GET, SET and partial update via SET operations. Other JSON commands supported by redis can be added as well. Kindly take a look and let me know if i should go ahead with the implementation with ajson.

I think we can proceed with Arpit's suggestion here. Let's pick up the feature extension work (path, performance) in a separate PR where we can evaluate our different options better. Let's also discuss that in the upcoming weekly sync.

Please proceed with implementing this as a map for now.

JyotinderSingh avatar Jul 30 '24 12:07 JyotinderSingh

@JyotinderSingh I have gone with interface{} for storing the json in our application as redis' JSON.SET supports primitive datatypes as well:

JSON.SET message $ '"Hello, World!"'

JSON.SET booleanTrue $ true

JSON.SET numbers $ '[1,2,3,4,5]'

Support for these would not have been possible by having map[string]interface{}. Do let me know if I am missing something. All tests are passing and I also have a python script for testing out the flow of SET and GET operations.

I am very close to finishing the #166 using ajson, as a POC.

Maveric-k07 avatar Jul 31 '24 14:07 Maveric-k07

@JyotinderSingh I have gone with interface{} for storing the json in our application as redis' JSON.SET supports primitive datatypes as well:

JSON.SET message $ '"Hello, World!"'

JSON.SET booleanTrue $ true

JSON.SET numbers $ '[1,2,3,4,5]'

Support for these would not have been possible by having map[string]interface{}. Do let me know if I am missing something. All tests are passing and I also have a python script for testing out the flow of SET and GET operations.

I am very close to finishing the #166 using ajson, as a POC.

Thanks for the changes. It makes sense to use an interface{}. Overall the PR looks good.

Also, I'm not sure why a python script is needed to test the operations, we already have the required go infrastructure for E2E testing in our repo.

Have also left a few comments.

JyotinderSingh avatar Jul 31 '24 16:07 JyotinderSingh

Also, I'm not sure why a python script is needed to test the operations, we already have the required go infrastructure for E2E testing in our repo.

@JyotinderSingh The python script is temporary dev script that i am using for testing various cases in the JSON.SET implementation, will not be added to the repo. Had to rebase the branch, apologies.

I have made changes to address your suggestions above. Please check. Thanks

Maveric-k07 avatar Aug 01 '24 07:08 Maveric-k07