You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/11/13 06:02:12 UTC
[GitHub] [incubator-kvrocks] boatrainlsz opened a new pull request, #1116: feat: add support for Redis sintercard command
boatrainlsz opened a new pull request, #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116
add support for Redis sintercard command, solve #1083
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1312651487
Thank you very much for your contribution. Maybe you also need to add the go test, you can refer to `incubator-kvrocks/tests/gocase/unit/type/set/set_test.go`
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1116: feat: add support for Redis sintercard command
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1552872079
Close this PR due to out-of-date and no response.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020855635
##########
src/commands/redis_cmd.cc:
##########
@@ -2270,6 +2277,54 @@ class CommandSInter : public Commander {
}
};
+class CommandSInterCard : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ // args_ will be like: sintercard 2 key1 key2 LIMIT 1, where 2 is the number of keys, limit is optional
+ uint64_t num_keys = 0;
+ auto parse_num_keys = ParseInt<uint64_t>(args_[1], 10);
+ if (!parse_num_keys) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ num_keys = *parse_num_keys;
+ if (num_keys <= 0) {
+ return Status(Status::NotOK, "numkeys should be greater than 0");
+ }
+ if (args_.size() < num_keys + 2) {
+ return Status(Status::NotOK, "numkeys should be equal to the number of keys");
+ }
+ if (args_.size() != num_keys + 2 && args_.size() != num_keys + 4) {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ int64_t limit = 0;
+ if (args_.size() == num_keys + 4) {
+ if (Util::ToLower(args_[num_keys + 2]) != "limit") {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ auto parse_limit = ParseInt<int64_t>(args_[num_keys + 3], 10);
+ if (!parse_limit) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ limit = *parse_limit;
+ if (limit < 0) {
+ return Status(Status::NotOK, "limit can't be negative");
+ }
+ }
+ std::vector<Slice> keys;
+ for (uint64_t i = 2; i < num_keys + 2; i++) {
+ keys.emplace_back(args_[i]);
+ }
+ Redis::Set set_db(svr->storage_, conn->GetNamespace());
+ int64_t ret = 0;
+ auto s = set_db.InterCard(keys, limit, &ret);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::Integer(ret);
+ return Status::OK();
+ }
+};
Review Comment:
It is optional to use the command parser, but I think at least we need to extract the parsing part in this method `Execute` to `Parse`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] boatrainlsz commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
boatrainlsz commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020850182
##########
src/commands/redis_cmd.cc:
##########
@@ -2270,6 +2277,54 @@ class CommandSInter : public Commander {
}
};
+class CommandSInterCard : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ // args_ will be like: sintercard 2 key1 key2 LIMIT 1, where 2 is the number of keys, limit is optional
+ uint64_t num_keys = 0;
+ auto parse_num_keys = ParseInt<uint64_t>(args_[1], 10);
+ if (!parse_num_keys) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ num_keys = *parse_num_keys;
+ if (num_keys <= 0) {
+ return Status(Status::NotOK, "numkeys should be greater than 0");
+ }
+ if (args_.size() < num_keys + 2) {
+ return Status(Status::NotOK, "numkeys should be equal to the number of keys");
+ }
+ if (args_.size() != num_keys + 2 && args_.size() != num_keys + 4) {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ int64_t limit = 0;
+ if (args_.size() == num_keys + 4) {
+ if (Util::ToLower(args_[num_keys + 2]) != "limit") {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ auto parse_limit = ParseInt<int64_t>(args_[num_keys + 3], 10);
+ if (!parse_limit) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ limit = *parse_limit;
+ if (limit < 0) {
+ return Status(Status::NotOK, "limit can't be negative");
+ }
+ }
+ std::vector<Slice> keys;
+ for (uint64_t i = 2; i < num_keys + 2; i++) {
+ keys.emplace_back(args_[i]);
+ }
+ Redis::Set set_db(svr->storage_, conn->GetNamespace());
+ int64_t ret = 0;
+ auto s = set_db.InterCard(keys, limit, &ret);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::Integer(ret);
+ return Status::OK();
+ }
+};
Review Comment:
@tanruixiang Thanks for advice.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020855423
##########
tests/cppunit/t_set_test.cc:
##########
@@ -188,6 +188,27 @@ TEST_F(RedisSetTest, Inter) {
set->Del(k3);
}
+TEST_F(RedisSetTest, InterCard) {
+ int ret;
Review Comment:
Would you like to also write some unit tests in golang? e.g. `tests/gocase/unit/type/...`
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1328009997
Code conflict as we did some refactors. cc @torwig @PragmaTwice could you help with providing the plan on following refactors so that we don't resolve conflicts many times?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1328028234
@tisonkun Resolved. However, this feature is not ready to be merged.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020849652
##########
src/commands/redis_cmd.cc:
##########
@@ -2270,6 +2277,54 @@ class CommandSInter : public Commander {
}
};
+class CommandSInterCard : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ // args_ will be like: sintercard 2 key1 key2 LIMIT 1, where 2 is the number of keys, limit is optional
+ uint64_t num_keys = 0;
+ auto parse_num_keys = ParseInt<uint64_t>(args_[1], 10);
+ if (!parse_num_keys) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ num_keys = *parse_num_keys;
+ if (num_keys <= 0) {
+ return Status(Status::NotOK, "numkeys should be greater than 0");
+ }
+ if (args_.size() < num_keys + 2) {
+ return Status(Status::NotOK, "numkeys should be equal to the number of keys");
+ }
+ if (args_.size() != num_keys + 2 && args_.size() != num_keys + 4) {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ int64_t limit = 0;
+ if (args_.size() == num_keys + 4) {
+ if (Util::ToLower(args_[num_keys + 2]) != "limit") {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ auto parse_limit = ParseInt<int64_t>(args_[num_keys + 3], 10);
+ if (!parse_limit) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ limit = *parse_limit;
+ if (limit < 0) {
+ return Status(Status::NotOK, "limit can't be negative");
+ }
+ }
+ std::vector<Slice> keys;
+ for (uint64_t i = 2; i < num_keys + 2; i++) {
+ keys.emplace_back(args_[i]);
+ }
+ Redis::Set set_db(svr->storage_, conn->GetNamespace());
+ int64_t ret = 0;
+ auto s = set_db.InterCard(keys, limit, &ret);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::Integer(ret);
+ return Status::OK();
+ }
+};
Review Comment:
You can refer to #1032 to use a more concise parsing approach.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020855261
##########
src/commands/redis_cmd.cc:
##########
@@ -2270,6 +2277,54 @@ class CommandSInter : public Commander {
}
};
+class CommandSInterCard : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
Review Comment:
I think you can try to split this method to two methods: `Parse` and `Execute`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] boatrainlsz commented on pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
boatrainlsz commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1312704338
Everybody, Thank you for all your suggestions, I will recheck related codes.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1116: feat: add support for Redis sintercard command
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1549740422
> @PragmaTwice Can I fix this pr because of long-term loss of contact? I don't know how to collaborate with this.
Sure. I think you can pick up this PR.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk closed pull request #1116: feat: add support for Redis sintercard command
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk closed pull request #1116: feat: add support for Redis sintercard command
URL: https://github.com/apache/incubator-kvrocks/pull/1116
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1116: feat: add support for Redis sintercard command
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1549550742
@PragmaTwice Can I fix this pr because of long-term loss of contact? I don't know what I need to collaborate with this.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1116: feat: add support for Redis sintercard command
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#issuecomment-1480612083
Hi @boatrainlsz , do you still have time to continue this PR?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020855635
##########
src/commands/redis_cmd.cc:
##########
@@ -2270,6 +2277,54 @@ class CommandSInter : public Commander {
}
};
+class CommandSInterCard : public Commander {
+ public:
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ // args_ will be like: sintercard 2 key1 key2 LIMIT 1, where 2 is the number of keys, limit is optional
+ uint64_t num_keys = 0;
+ auto parse_num_keys = ParseInt<uint64_t>(args_[1], 10);
+ if (!parse_num_keys) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ num_keys = *parse_num_keys;
+ if (num_keys <= 0) {
+ return Status(Status::NotOK, "numkeys should be greater than 0");
+ }
+ if (args_.size() < num_keys + 2) {
+ return Status(Status::NotOK, "numkeys should be equal to the number of keys");
+ }
+ if (args_.size() != num_keys + 2 && args_.size() != num_keys + 4) {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ int64_t limit = 0;
+ if (args_.size() == num_keys + 4) {
+ if (Util::ToLower(args_[num_keys + 2]) != "limit") {
+ return Status(Status::NotOK, "invalid number of arguments");
+ }
+ auto parse_limit = ParseInt<int64_t>(args_[num_keys + 3], 10);
+ if (!parse_limit) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ limit = *parse_limit;
+ if (limit < 0) {
+ return Status(Status::NotOK, "limit can't be negative");
+ }
+ }
+ std::vector<Slice> keys;
+ for (uint64_t i = 2; i < num_keys + 2; i++) {
+ keys.emplace_back(args_[i]);
+ }
+ Redis::Set set_db(svr->storage_, conn->GetNamespace());
+ int64_t ret = 0;
+ auto s = set_db.InterCard(keys, limit, &ret);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::Integer(ret);
+ return Status::OK();
+ }
+};
Review Comment:
It is optional to use the command parser, but I think at least we need extract the parsing part in this method `Execute` to `Parse`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1116: feat: add support for Redis sintercard command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020872184
##########
src/types/redis_set.cc:
##########
@@ -133,6 +133,17 @@ rocksdb::Status Set::Card(const Slice &user_key, int *ret) {
return rocksdb::Status::OK();
}
+rocksdb::Status Set::InterCard(const std::vector<Slice> &keys, int64_t limit, int64_t *ret) {
+ std::vector<std::string> members;
+ rocksdb::Status s = Inter(keys, &members);
+ if (!s.ok()) return s;
+ *ret = members.size();
+ if (limit > 0 && *ret > limit) {
Review Comment:
The Redis docs states:
`By default, the command calculates the cardinality of the intersection of all given sets. When provided with the optional LIMIT argument (which defaults to 0 and means unlimited), if the intersection cardinality reaches limit partway through the computation, the algorithm will exit and yield limit as the cardinality. Such implementation ensures a significant speedup for queries where the limit is lower than the actual intersection cardinality.`
I think, your implementation doesn't speed up the process of intersection, it performs an intersection as usual (full) and adjusts the result by comparing it with the `limit` value. In order to truly use the `limit` value, you should use it somewhere inside `Inter()`, passing it as a parameter or introducing a completely separate method.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org