ValkeyJSON RFC

[joehu21]

The proposed Valkey JSON module, named ValkeyJSON, supports the native JavaScript Object Notation (JSON) format to encode complex datasets inside Valkey. It is compliant with RFC7159 and ECMA-404 JSON data interchange standard. With this feature, users can natively store, query, and modify JSON data structures in ValKey using the popular JSONPath query language. To help users migrate from Redis and RedisJSON, as well as capitalize on existing OSS RedisJSON client libraries, the module is designed to be API-compatible and RDB-compatible with Redis Ltd.’s RedisJSON v2.

[zuiderkwast]

Thanks!

The content looks very good in general, but I think some should be moved from design considerations to specification. I don't know if others agree. The RFC process is new for all of us.

[zuiderkwast]

One more thing: Valkey is spelled with lowercase "k". 😄

[joehu21]

Updated all occurrences of "ValKey" to "Valkey", including the work in graphs.

[zuiderkwast]

LGTM

[zuiderkwast]

@madolson do you want to take a quick look before we merge this?

[madolson]

@zuiderkwast since it becomes harder to comment after it's merged. I thought we should let everyone comment first before merging it as a proposal.

[zuiderkwast]

@zuiderkwast since it becomes harder to comment after it's merged. I thought we should let everyone comment first before merging it as a proposal.

Sounds reasonable. If this is the process we want, we should probably clarify that in the README at some point. The README says we'll merge the PR once the style and format is fine.

[bbarani]

@madolson @daniel-house @zuiderkwast Whats the next step here? Are we good to merge this PR?

[madolson]

I would like to know why not have two separate commands, JSON.DEPTH and JSON.FIELDs ? Having a debug command doesn't really make that much sense.

[joehu21]

The intent is to avoid diverging from RedisJSON unnecessarily. If we introduce new commands, they won't be supported by the existing OSS RedisJSON client libraries. If we have our own ValkeyJSON client library ecosystem, then it would make sense to have them as new commands, instead of sub-commands of JSON.DEBUG.

[joehu21]

In the RFC, “JSON.DEBUG DEPTH” and “JSON.DEBUG FIELDS” are the only extension. We consider "depth of a json tree/sub-tree" and "number of fields in a json tree/sub-tree" as debug information. Hence, they are subcommands of JSON.DEBUG. However, it doesn't mean in the future, all extensions should be subcommands of JSON.DEBUG. I am open to adding new JSON commands in the future if necessary.

[daniel-house]

I tried to look up the list of commands elsewhere and my best source for the RedisJSON v2 API didn't agree with this. Probably I am not looking in the right place. Please point to it for me.

I saw JSON.DEBUG without arguments (which is not in the PR), and JSON.DEBUG MEMORY (which is), but not JSON.DEBUG HELP (an extension?). I also saw JSON.MSET (an oversight?).

[madolson]

I also saw JSON.MSET (an oversight?)

Yeah, we missed this. This was added sort of randomly by Redis at some point.

[joehu21]

RedisJSON API is evolving. Missed JSON.MSET. Will add it to the RFC when I get a chance.

Interestingly, their documentation doesn't seem to match the latest docker image redis/redis-stack:latest. I don't see json.mset:

docker run -p 6379:6379 redis/redis-stack:latest
...
% ./redis-cli command |grep -i json
json.arrtrim
json.arrpop
json.clear
json.arrappend
json.objlen
json.nummultby
json.mget
json.strappend
json.toggle
json.get
json.arrlen
json.strlen
json.numpowby
json.resp
json.type
json.forget
json.arrindex
json.arrinsert
json.set
json.del
json.numincrby
json.debug
json.objkeys

I saw JSON.DEBUG without arguments (which is not in the PR), and JSON.DEBUG MEMORY (which is), but not JSON.DEBUG HELP (an extension?).

That's not what I see in the docker image. Using the same docker image, I see JSON.DEBUG HELP is valid and JSON.DEBUG is invalid. I think this behavior matches their API doc of JSON.DEBUG - "This is a container command for debugging related tasks." My understanding is that "container command" doesn't necessarily mean JSON.DEBUG without arguments is a valid command.

127.0.0.1:6379> json.debug help
1) "MEMORY <key> [path] - reports memory usage"
2) "HELP                - this message"
127.0.0.1:6379> json.debug
(error) ERR wrong number of arguments for 'json.debug' command

[joehu21]

Just realized the docker image above has json.numpowby which I was not aware of. However, I don't see this command in RedisJSON API documentation. Not sure what we'll do with it.

Anyway, we do observe the following with RedisJSON:

1. Their published API may not match the latest docker binary.
2. Occasionally they add some new commands.
3. OSS JSON client libraries may not match the latest API doc and/or docker binary.

[madolson]

I have a feeling it might be deprecated. I found out the json.nummultiply is deprecated as per https://github.com/redis/redis-py/blob/1cbb97ecd7a0bf87aec332a257b589919385d90d/redis/commands/json/commands.py#L144C6-L144C25.

[madolson]

This RFC looks good to me as well. As per our updated guidance, @valkey-io/core-team please take a look and review offline. We'll schedule to review this next week in our team meeting: 2024-08-12. I'll send an invite to Joe.

[enjoy-binbin]

i did not review all the lines carefully, LGTM with a rough look.