JSON Module rewrite with custom JSONPath implementation #974
Conversation
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\libs\server\Garnet.server.csproj" /> | ||
| <InternalsVisibleTo Include="Garnet.test" Key="0024000004800000940000000602000000240000525341310004000001000100011b1661238d3d3c76232193c8aa2de8c05b8930d6dfe8cd88797a8f5624fdf14a1643141f31da05c0f67961b0e3a64c7120001d2f8579f01ac788b0ff545790d44854abe02f42bfe36a056166a75c6a694db8c5b6609cff8a2dbb429855a1d9f79d4d8ec3e145c74bfdd903274b7344beea93eab86b422652f8dd8eecf530d2" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Instead of playground, can we use a top level folder called modules and put this in there?
|
Edit: Ran |
|
@darrenge Add expected values in BDN_Benchmark_Config.json |
There was a problem hiding this comment.
What an extensive effort! Nicely done @Vijay-Nirmal. I've added some comments. I haven't looked deeply into the unmitigated issues yet (custom writer & null sets), I'll look at those next and give my opinion...
| EndGlobalSection | ||
| GlobalSection(ExtensibilityGlobals) = postSolution | ||
| FileExplorer = |
There was a problem hiding this comment.
This looks weird, should this be here?
|
|
||
| private readonly Consumer _consumer = new Consumer(); | ||
|
|
||
| [Params( |
There was a problem hiding this comment.
General Q - where did these test cases come from? Just curious if this is copied from somewhere.
| "$.store.book[*].author", | ||
| "$.store.book[?(@.price < 10)].title", | ||
| "$.store.bicycle.color", | ||
| "$.store.book[*]", // all books |
There was a problem hiding this comment.
nit: indents are wonky, just replace with a single space
| [MemoryDiagnoser] | ||
| public class JsonOperations : OperationsBase | ||
| { | ||
| // Existing commands |
There was a problem hiding this comment.
nit: Existing & new with regards to what? These would all be existing commands once the PR is merged...
|
|
||
| private void RegisterModules() | ||
| { | ||
| server.Register.NewModule(new NoOpModule.NoOpModule(), [], out _); |
There was a problem hiding this comment.
Why is the No-op module needed here?
| JsonPath jsonPath = new JsonPath(pathStr); | ||
| var result = jsonPath.Evaluate(rootNode, rootNode, null).ToArray(); | ||
|
|
||
| if (!result.Any()) |
There was a problem hiding this comment.
Checking if Length is 0 would be more performant here.
| break; | ||
| } | ||
|
|
||
| // Known issue: When the value to be replaced is null, replace won't work as there is no NullJsonValue, instead .net returns null |
There was a problem hiding this comment.
I would add this to the XML comment as well.
| { | ||
| return array; | ||
| } | ||
| else |
| return output.AbortWithErrorMessage(CmdStrings.RESP_SYNTAX_ERROR); | ||
| } | ||
| } | ||
| var result = ((GarnetJsonObject)jsonObject).Set(path, value, existOptions, out var errorMessage); |
There was a problem hiding this comment.
nit: you can do: var garnetJsonObject = jsonObject as GarnetJsonObject; Debug.Assert(garnetJsonObject != null); at the beginning instead of doing a type check + cast (same in JsonGET)
| var option = CustomCommandUtils.GetNextArg(ref input, ref offset); | ||
| if (option.EqualsUpperCaseSpanIgnoringCase(JsonCmdStrings.INDENT) && offset < parseState.Count) | ||
| { | ||
| indent = Encoding.UTF8.GetString(CustomCommandUtils.GetNextArg(ref input, ref offset)); |
There was a problem hiding this comment.
Can add a GetNextString to CustomObjectFunctions
|
Thoughts about INDENT / NEWLINE / SPACE...
|
As part of this PR, the JSON module implementation has been rewritten using a custom implementation of JsonPath to achieve better performance and compatibility with Redis. As part of this rewrite other parts of the Json module have also been rewritten
Main Changes:
JsonPath.Netpackage referenceMiscellaneous:
CmdStringsclass public inCmdStrings.csto allow external access.ExistOptionsenum to public inRespEnums.csfor broader accessibility.Known Issues:
nullusingJSON.SETcommand then the update will fail, it can't find the parent object to update the value. This is a limitation of the custom JSONPath implementation because of the way the Null is represented in JsonNode. Will find a way to support this in future without affecting the performance much. This issue shouldn't block the PR from being merged as this scenario was failing in the current implementation as well.Utf8JsonWriterdoesn't allow enough customizability to supportINDENT,NEWLINEandSPACEoptions in JSON.GET command. We can't inherit the Utf8JsonWriter class as well as it's a sealed class to write our own logic to customize it. This feature will not be supported in STJ in future as well [API Proposal]: Allow inheritance for Utf8JsonWriter class dotnet/runtime#111899 (comment). We have only two options, either Garnet can never support the customizability provided by Redis for certain JSON commands, or Garnet should use Newtonsoft.Json (which supports customisation). Both of these options are not ideal.TODO
Custom JSONPath Benchmark
Note and a disclaimer: JsonCraft.JsonPath is a package I published. If you need Garent's implementation as a separate package you can use JsonCraft.JsonPath package. In the benchmark, it looks slightly slower because it's benchmarked against the
JsonElementimplementation instead ofJsonNode. You can find the exact same implementations as Garent in Experimental folderGarnet BDN Benchmark
In the main branch as of 762a9d7
In this PR as of 7650f5f: (More improvements to come)
Out of scope of this PR
Some of these items I will be raising future PRs to address it
/