mapleFU commented on code in PR #1841:
URL: https://github.com/apache/kvrocks/pull/1841#discussion_r1366499172
##########
tests/gocase/unit/type/json/json_test.go:
##########
@@ -34,7 +35,7 @@ func TestJson(t *testing.T) {
rdb := srv.NewClient()
defer func() { require.NoError(t, rdb.Close()) }()
- t.Run("JSON.SET and JSON.GET basics", func(t *testing.T) {
+ t.Run("JSON.SET and JSON.GET and JSON.STRAPPEND basics", func(t
*testing.T) {
Review Comment:
You'd better start a new test here?
```
t.Run("JSON.STRAPPEND tests", func(t *testing.T) {})
```
##########
src/types/redis_json.cc:
##########
@@ -117,4 +117,30 @@ rocksdb::Status Json::Get(const std::string &user_key,
const std::vector<std::st
return rocksdb::Status::OK();
}
+rocksdb::Status Json::StrAppend(const std::string &user_key, const
std::vector<std::string> &paths, const std::string &value, uint64_t
&deleted_cnt) {
+ deleted_cnt = 0;
+ auto ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard(storage_->GetLockManager(), ns_key);
+
+ std::string bytes;
+ JsonMetadata metadata;
+ Slice rest;
+ auto s = GetMetadata(kRedisJson, ns_key, &bytes, &metadata, &rest);
+ if (!s.ok()) return s;
+
+ if (metadata.format != JsonStorageFormat::JSON)
+ return rocksdb::Status::NotSupported("JSON storage format not supported");
+
+ auto json_res = JsonValue::FromString(rest.ToStringView());
+ if (!json_res) return rocksdb::Status::Corruption(json_res.Msg());
+ auto json_val = *std::move(json_res);
+
+ for (const auto &path : paths) {
+ auto append_res = json_val.Append(path, value, deleted_cnt);
+ if (!append_res) return rocksdb::Status::InvalidArgument(append_res.Msg());
+ }
+ return write(ns_key, &metadata, json_val);
Review Comment:
Can we avoid a `write` when nothing changed in underlying json (.i.e
`deleted_count == 0` in your case )
##########
src/commands/cmd_json.cc:
##########
@@ -52,7 +52,33 @@ class CommandJsonGet : public Commander {
}
};
+class CommandJsonStrAppend : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output)
override {
+ redis::Json json(svr->storage, conn->GetNamespace());
+
+ JsonValue result;
+ std::vector<std::string> paths;
+ std::vector<std::string> values;
Review Comment:
Seems that `path` and `value` should be `std::string`?
You can also take a look at `CommandParser`.
```
std::string path_or_root = "$";
std::string value;
if (args_.size() == 3) {
value = ...
} else {
path =
value =
}
```
##########
src/commands/cmd_json.cc:
##########
@@ -52,7 +52,33 @@ class CommandJsonGet : public Commander {
}
};
+class CommandJsonStrAppend : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output)
override {
+ redis::Json json(svr->storage, conn->GetNamespace());
+
+ JsonValue result;
+ std::vector<std::string> paths;
+ std::vector<std::string> values;
+ for (int i=2;i<(int)args_.size();++i) {
+ auto val = args_[i];
+ if (val.substr(0, 1) == "$") {
+ paths.push_back(val);
+ } else{
+ values.push_back(val);
+ }
+ }
+ uint64_t cnt = 0;
+ auto s = json.StrAppend(args_[1], {args_.begin() + 2, args_.end() - 1},
args_.back(), cnt);
+ if (!s.ok()) return {Status::RedisExecErr, s.ToString()};
+
+ *output = redis::Integer(cnt);
+ return Status::OK();
+ }
+};
+
REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandJsonSet>("json.set", -3, "write",
1, 1, 1),
- MakeCmdAttr<CommandJsonGet>("json.get", -2,
"read-only", 1, 1, 1), );
+ MakeCmdAttr<CommandJsonGet>("json.get", -2,
"read-only", 1, 1, 1),
+ MakeCmdAttr<CommandJsonStrAppend>("json.strappend",
-2, "read-only", 1, 1, 1), );
Review Comment:
I don't know why arity is -2. The syntax for it is:
```
JSON.STRAPPEND key [path] value
```
Seems that the arity should be `-3` since it should have at lease
`JSON.STRAPPEND, key, value` ?
##########
src/commands/cmd_json.cc:
##########
@@ -52,7 +52,33 @@ class CommandJsonGet : public Commander {
}
};
+class CommandJsonStrAppend : public Commander {
+public:
+ Status Execute(Server *svr, Connection *conn, std::string *output)
override {
+ redis::Json json(svr->storage, conn->GetNamespace());
+
+ JsonValue result;
+ std::vector<std::string> paths;
+ std::vector<std::string> values;
+ for (int i=2;i<(int)args_.size();++i) {
+ auto val = args_[i];
+ if (val.substr(0, 1) == "$") {
+ paths.push_back(val);
+ } else{
+ values.push_back(val);
+ }
+ }
+ uint64_t cnt = 0;
Review Comment:
The return value should be an array of int like
https://github.com/apache/kvrocks/pull/1837 rather than a int
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
