feat: add support of the command JSON.DEBUG MEMORY #2323

jackyyyyyssss · 2024-05-20T10:38:00Z

Add support of the command JSON.DEBUG MEMORY
https://redis.io/docs/latest/commands/json.debug-memory/

mapleFU

Another problem is that, should we emit the memory size ( And memory size might larger than str size ) or just export the size on disk?

mapleFU · 2024-05-20T10:40:37Z

src/commands/cmd_json.cc

+    std::string path = "$";
+
+    if (!util::EqualICase(args_[1], "memory")) {
+      return {Status::RedisExecErr, "The number of arguments is more than expected"};


why this is number more than expected?

mapleFU · 2024-05-20T10:41:21Z

src/types/redis_json.cc

@@ -626,5 +626,17 @@ std::vector<rocksdb::Status> Json::readMulti(const std::vector<Slice> &ns_keys,
  }
  return statuses;
 }
+rocksdb::Status Json::DebugMemory(const std::string &user_key, const std::string &path, Optionals<uint64_t> *results) {


format or adding a space between them?

mapleFU · 2024-05-20T10:47:34Z

src/types/json.h

+            if (!origin.empty()) {
+              // This is only a rough calculation of the size of the string size char, not the entire byte occupied by
+              // the object
+              results.emplace_back(origin.as_string().size() * sizeof(std::string::value_type));


Could this json_query matches more than 1 element?

If multiple elements are hit, an array is returned. This is the return value of Redis 7.0

PragmaTwice

There are several issues in this implemetation:

the result of as_string().size() is not equal to the real size, we should at least use to_string()
but actually to_string is also wrong, since it maybe not equal to the result of compact_json_string_encoder. also it would be totally wrong if CBOR format is enabled.
there may be multiple location found from one jsonpath, which is not handled in this impl
if jsonpath is $, no json decoding is required

jackyyyyyssss · 2024-05-21T09:23:46Z

There are several issues in this implemetation:

the result of as_string().size() is not equal to the real size, we should at least use to_string()

but actually to_string is also wrong, since it maybe not equal to the result of compact_json_string_encoder. also it would be totally wrong if CBOR format is enabled.

there may be multiple location found from one jsonpath, which is not handled in this impl

if jsonpath is $, no json decoding is required

Thank you for your help. The modifications have been completed, please help me take a look again

PragmaTwice · 2024-05-21T10:33:28Z

src/types/json.h

+          value, path,
+          [&results, max_nesting_depth, format](const std::string & /*path*/, const jsoncons::json &origin) {
+            if (!origin.empty()) {
+              Status s;


Is it useful here?

PragmaTwice · 2024-05-21T10:33:58Z

src/types/json.h

+                origin.dump(encoder, ec);
+              } else if (format == JsonStorageFormat::CBOR) {
+                jsoncons::cbor::cbor_options options;
+                options.max_nesting_depth(max_nesting_depth);
+                jsoncons::cbor::basic_cbor_encoder<jsoncons::string_sink<std::string>> encoder{buffer, options};
+                origin.dump(encoder, ec);


Please use the exception version of dump and catch it later.

PragmaTwice · 2024-05-21T10:34:31Z

src/types/json.h

@@ -217,6 +217,42 @@ struct JsonValue {
    return results;
  }

+  StatusOr<Optionals<uint64_t>> StrBytes(std::string_view path, JsonStorageFormat format,


Is optional useful here? Could we just use vector?

PragmaTwice · 2024-05-21T10:35:30Z

src/types/json.h

@@ -217,6 +217,42 @@ struct JsonValue {
    return results;
  }

+  StatusOr<Optionals<uint64_t>> StrBytes(std::string_view path, JsonStorageFormat format,


Suggested change

StatusOr<Optionals<uint64_t>> StrBytes(std::string_view path, JsonStorageFormat format,

StatusOr<Optionals<uint64_t>> GetBytes(std::string_view path, JsonStorageFormat format,

PragmaTwice · 2024-05-21T10:35:56Z

src/types/redis_json.cc

+    Slice rest;
+    auto s = GetMetadata(GetOptions{}, {kRedisJson}, ns_key, &bytes, &metadata, &rest);
+    if (!s.ok()) return s;
+    results->emplace_back(bytes.size());


Suggested change

results->emplace_back(bytes.size());

results->emplace_back(rest.size());

PragmaTwice · 2024-05-22T03:30:48Z

src/types/json.h

@@ -217,6 +217,42 @@ struct JsonValue {
    return results;
  }

+  StatusOr<std::vector<std::string>> GetBytes(std::string_view path, JsonStorageFormat format,


Why vector<string> instead of vector<size_t>?

Because currently ArrayOfBulkStrings only supports string types, if modified to a template, it cannot meet the writing requirements of std:: initializer_list. Do you suggest that I add another method, std: vector<size_t>?

You can use a loop to dump RESP output. It doesn't matter whether you use ArrayOfBulkStrings or not.

PragmaTwice · 2024-05-22T03:33:09Z

src/types/json.h

+    try {
+      jsoncons::jsonpath::json_query(value, path, [&](const std::string & /*path*/, const jsoncons::json &origin) {
+        if (!origin.empty()) {
+          Status s;


Is s really useful here? (I have to ask again)

This function is used to traverse the internal structure of JSON and will loop through the callback function multiple times inside

I mean the variable s.

PragmaTwice · 2024-05-22T03:33:34Z

src/types/json.h

+          if (format == JsonStorageFormat::JSON) {
+            jsoncons::json_options options;
+            options.max_nesting_depth(max_nesting_depth);
+            jsoncons::compact_json_string_encoder encoder{buffer, options};
+            origin.dump(encoder, ec);
+          } else if (format == JsonStorageFormat::CBOR) {
+            jsoncons::cbor::cbor_options options;
+            options.max_nesting_depth(max_nesting_depth);
+            jsoncons::cbor::basic_cbor_encoder<jsoncons::string_sink<std::string>> encoder{buffer, options};
+            origin.dump(encoder, ec);
+          }


Please use Dump and DumpCBOR.

PragmaTwice · 2024-05-22T03:33:43Z

src/types/json.h

+          if (ec) {
+            throw std::system_error(ec);
+          }


Please remove it.

PragmaTwice · 2024-05-22T03:35:56Z

tests/gocase/unit/type/json/json_test.go

+		var result2 = make([]interface{}, 0)
+		result2 = append(result2, int64(4), int64(6), int64(2))
+		require.Equal(t, rdb.Do(ctx, "JSON.DEBUG", "MEMORY", "a", "$..a").Val(), result2)
+		require.ErrorIs(t, rdb.Do(ctx, "JSON.DEBUG", "MEMORY", "not_exists", "$").Err(), redis.Nil)


Could you add more test cases? e.g. for a valid $, or a number/array/object, or a non-existent path.

Okay, I will add all the testcases

PragmaTwice · 2024-05-22T03:44:33Z

src/types/json.h

+    std::vector<std::string> results;
+    try {
+      jsoncons::jsonpath::json_query(value, path, [&](const std::string & /*path*/, const jsoncons::json &origin) {
+        if (!origin.empty()) {


What's the meaning of this empty check?
Could you think about whether an empty JSON array/object will occupy storage space?

Empty arrays and nulls actually take up space, and the test cases have been supplemented. Please see if there are any areas that can be optimized

PragmaTwice · 2024-05-22T15:16:09Z

src/commands/cmd_json.cc

@@ -45,6 +45,14 @@ std::string OptionalsToString(const Connection *conn, Optionals<T> &opts) {
  return str;
 }

+std::string SizeToString(const std::vector<std::size_t> &elems) {
+  std::string result = "*" + std::to_string(elems.size()) + CRLF;


You can use MultiLen here.

PragmaTwice · 2024-05-22T15:21:02Z

src/commands/cmd_json.cc

+    auto s = json.DebugMemory(args_[2], path, &results);
+
+    if (s.IsNotFound()) {
+      *output = conn->NilString();


It's wrong. For nonexistent key, the result is 0.

Please confirm with redis-stack and redis-cli by yourself.

Thank you again. The latest version of Redis Stack returns all cases, and I will make the necessary modifications later

PragmaTwice · 2024-05-22T15:21:36Z

src/types/json.h

+    if (results.size() == 0) {
+      results.emplace_back(0);
+    }


It's wrong.

Please use redis-stack and redis-cli to confirm the correct result for this case.

PragmaTwice · 2024-05-27T14:27:22Z

src/types/json.h

+    std::vector<size_t> results;
+    Status s;
+    try {
+      jsoncons::jsonpath::json_query(value, path, [&](const std::string & /*path*/, const jsoncons::json &origin) {


Suggested change

jsoncons::jsonpath::json_query(value, path, [&](const std::string & /*path*/, const jsoncons::json &origin) {

jsoncons::jsonpath::json_query(value, path, [&](const std::string & /*path*/, const jsoncons::json &origin) {

if (!s) return;

Hello, I have two questions I would like to ask. 1. When does the status initialization fail and needs to be returned in advance? 2. Currently, obtaining the byte is still inaccurate, and the actual value should be larger than the encoded string. size(). Does this meet the project's standards？

There's no initialization failure.

We should check it since the lambda function can be executed more than once.

PragmaTwice · 2024-05-27T14:29:36Z

src/commands/cmd_json.cc

@@ -647,6 +689,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandJsonSet>("json.set", 4, "write", 1, 1
                        MakeCmdAttr<CommandJsonStrAppend>("json.strappend", -3, "write", 1, 1, 1),
                        MakeCmdAttr<CommandJsonStrLen>("json.strlen", -2, "read-only", 1, 1, 1),
                        MakeCmdAttr<CommandJsonMGet>("json.mget", -3, "read-only", 1, -2, 1),
-                        MakeCmdAttr<CommandJsonMSet>("json.mset", -4, "write", 1, -3, 3), );
+                        MakeCmdAttr<CommandJsonMSet>("json.mset", -4, "write", 1, -3, 3),
+                        MakeCmdAttr<CommandJsonDebug>("json.debug", -3, "read-only", 1, 2, 1));


Suggested change

MakeCmdAttr<CommandJsonDebug>("json.debug", -3, "read-only", 1, 2, 1));

MakeCmdAttr<CommandJsonDebug>("json.debug", -3, "read-only", 2, 2, 1));

src/commands/cmd_json.cc

PragmaTwice

hmm review comment still doesn't get solved yet

jackyyyyyssss · 2024-05-31T13:04:45Z

hmm review comment still doesn't get solved yet
I'm sorry I'm a bit clumsy, may I ask where I'm referring to

PragmaTwice · 2024-05-31T13:15:19Z

Ahh sorry I checked the diff again. It's fixed.

sonarqubecloud · 2024-05-31T14:35:26Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

q3356564 and others added 8 commits April 30, 2024 14:20

support DebugMemory

0e52a78

fix

0ba0fb5

Merge branch 'apache:unstable' into json-debug

f4005e4

Merge branch 'apache:unstable' into json-debug

15ed6fe

test support

c4d9759

fix

cb10eca

fix

d563233

fix

895ea71

mapleFU reviewed May 20, 2024

View reviewed changes

PragmaTwice requested changes May 20, 2024

View reviewed changes

fix

5a361a2

fix

4a3cb79

jackyyyyyssss marked this pull request as draft May 21, 2024 09:45

jackyyyyyssss marked this pull request as ready for review May 21, 2024 09:58

jackyyyyyssss marked this pull request as draft May 21, 2024 10:30

PragmaTwice reviewed May 21, 2024

View reviewed changes

q3356564 added 2 commits May 21, 2024 18:43

fix

e45dcd1

fix

e0e9faf

PragmaTwice reviewed May 22, 2024

View reviewed changes

src/types/json.h Outdated

Comment on lines 240 to 242

if (ec) {

throw std::system_error(ec);

}

Copy link

Member

PragmaTwice May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove it.

PragmaTwice reviewed May 22, 2024

View reviewed changes

fix

eb639b9

fix

a955aa8

jackyyyyyssss marked this pull request as ready for review May 22, 2024 08:29

PragmaTwice reviewed May 22, 2024

View reviewed changes

q3356564 added 5 commits May 23, 2024 11:09

fix case

15db3ef

fix

5fb1dac

fix empty

31ca58d

fix ci

847858e

fix zero

e0d69eb

jackyyyyyssss requested a review from PragmaTwice May 27, 2024 01:13

PragmaTwice reviewed May 27, 2024

View reviewed changes

q3356564 added 2 commits May 29, 2024 16:21

Suggested

29cda5d

fix

38521ca

git-hulk reviewed May 31, 2024

View reviewed changes

src/commands/cmd_json.cc Outdated Show resolved Hide resolved

fix

e4e4f80

jackyyyyyssss changed the title ~~Add support of the command JSON.DEBUG MEMORY~~ feat[json]: Add support of the command JSON.DEBUG MEMORY May 31, 2024

jackyyyyyssss changed the title ~~feat[json]: Add support of the command JSON.DEBUG MEMORY~~ feat[json]: add support of the command JSON.DEBUG MEMORY May 31, 2024

jackyyyyyssss changed the title ~~feat[json]: add support of the command JSON.DEBUG MEMORY~~ feat: add support of the command JSON.DEBUG MEMORY May 31, 2024

Merge branch 'unstable' into json-debug

9062108

jackyyyyyssss requested review from PragmaTwice and git-hulk May 31, 2024 11:37

PragmaTwice reviewed May 31, 2024

View reviewed changes

Merge branch 'unstable' into json-debug

e5de0c1

PragmaTwice approved these changes May 31, 2024

View reviewed changes

PragmaTwice merged commit 3557ad3 into apache:unstable May 31, 2024
32 checks passed

	StatusOr<Optionals<uint64_t>> StrBytes(std::string_view path, JsonStorageFormat format,
	StatusOr<Optionals<uint64_t>> GetBytes(std::string_view path, JsonStorageFormat format,

	results->emplace_back(bytes.size());
	results->emplace_back(rest.size());

	jsoncons::jsonpath::json_query(value, path, [&](const std::string & /path/, const jsoncons::json &origin) {
	jsoncons::jsonpath::json_query(value, path, [&](const std::string & /path/, const jsoncons::json &origin) {
	if (!s) return;

	MakeCmdAttr<CommandJsonDebug>("json.debug", -3, "read-only", 1, 2, 1));
	MakeCmdAttr<CommandJsonDebug>("json.debug", -3, "read-only", 2, 2, 1));

feat: add support of the command JSON.DEBUG MEMORY #2323

feat: add support of the command JSON.DEBUG MEMORY #2323

Conversation

jackyyyyyssss commented May 20, 2024

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice left a comment

Choose a reason for hiding this comment

jackyyyyyssss commented May 21, 2024

Choose a reason for hiding this comment

PragmaTwice May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyyyyyssss May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice left a comment

Choose a reason for hiding this comment

jackyyyyyssss commented May 31, 2024

PragmaTwice commented May 31, 2024

sonarqubecloud bot commented May 31, 2024

Quality Gate failed

PragmaTwice May 21, 2024 •

edited

Loading

jackyyyyyssss May 28, 2024 •

edited

Loading