You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2023/05/07 09:25:40 UTC
[GitHub] [incubator-kvrocks] tisonkun opened a new pull request, #1429: refactor: zset ranges
tisonkun opened a new pull request, #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429
This is a follow-up to #1428 for cleaning up some code.
--
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 #1429: refactor: zset ranges
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186849044
##########
src/types/redis_zset.cc:
##########
@@ -220,9 +219,12 @@ rocksdb::Status ZSet::Pop(const Slice &user_key, int count, bool min, std::vecto
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
-rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const CommonRangeRankSpec &spec,
- std::vector<MemberScore> *mscores) {
- mscores->clear();
+rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const RangeRankSpec &spec, MemberScores *mscores, int *ret) {
+ if (mscores) mscores->clear();
+
+ int cnt = 0;
+ if (!ret) ret = &cnt;
Review Comment:
Right, now I see.
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843450
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
OK. Let me set it to 0 and -1, correspondingly?
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186819875
##########
src/commands/cmd_zset.cc:
##########
@@ -356,59 +356,49 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- *output = redis::MultiBulkString(members, false);
+ if (!members.empty()) {
Review Comment:
Make sense. Let me think of how we can do this polymorphic thing properly...
--
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 #1429: refactor: zset ranges
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843071
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
We'd better to give default values to scalar types, otherwise the value is unspecified.
--
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 #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#issuecomment-1537451685
Merging...
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186819749
##########
src/commands/cmd_zset.cc:
##########
@@ -356,59 +356,49 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- *output = redis::MultiBulkString(members, false);
+ if (!members.empty()) {
Review Comment:
Below commands can reproduce this issue:
```
127.0.0.1:6666> zadd z1 1 a
(integer) 1
127.0.0.1:6666> zrange z1 2 3
```
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186821510
##########
src/commands/cmd_zset.cc:
##########
@@ -356,59 +356,49 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- *output = redis::MultiBulkString(members, false);
+ if (!members.empty()) {
Review Comment:
See https://github.com/apache/incubator-kvrocks/pull/1429/commits/1fffca8771c9d4e2da07f817dfa86fbfed8f746e
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186844938
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
Updated.
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843994
##########
src/commands/cmd_zset.cc:
##########
@@ -543,19 +528,22 @@ class CommandZRemRangeByScore : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- int size = 0;
redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- auto s = zset_db.RemoveRangeByScore(args_[1], spec_, &size);
+
+ int ret = 0;
Review Comment:
Let's use `cnt` here.
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186844367
##########
tests/cppunit/types/zset_test.cc:
##########
@@ -325,9 +328,18 @@ TEST_F(RedisZSetTest, RemoveRangeByRank) {
}
zset_->Add(key_, ZAddFlags::Default(), &mscores, &ret);
EXPECT_EQ(fields_.size(), ret);
- zset_->RemoveRangeByRank(key_, 0, static_cast<int>(fields_.size() - 2), &ret);
+
+ RangeRankSpec spec;
+ spec.removed = true;
Review Comment:
Make sense. I'll use `with_deletion` now.
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186819749
##########
src/commands/cmd_zset.cc:
##########
@@ -356,59 +356,49 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- *output = redis::MultiBulkString(members, false);
+ if (!members.empty()) {
Review Comment:
Below commands can reproduce this issue:
```
127.0.0.1:6666> zadd z1 1 a
(integer) 1
127.0.0.1:6666> zrange z1 2 3
```
The latter zrange command will block forever.
--
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] mapleFU commented on a diff in pull request #1429: refactor: zset ranges
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186841031
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
Seems that you set default min max for `RangeScoreSpec` in previous patch.
`RangeLexSpec`'s min max is `std::string`, they have default construct value "empty string".
Not setting is ok, but I wonder if start/stop be an undefined value here.
--
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 #1429: refactor: zset ranges
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843071
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
We'd better to give default values, otherwise the value is undetermined.
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
We'd better to give default values, otherwise the value is unspecified.
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186833194
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
It's somehow always set. But we can provide one?
If you take a closer look, `min` and `max` in the other two specs don't have default value also.
--
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] mapleFU commented on a diff in pull request #1429: refactor: zset ranges
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186831272
##########
src/common/range_spec.h:
##########
@@ -24,36 +24,36 @@
#include "status.h"
-struct CommonRangeLexSpec {
+struct RangeLexSpec {
std::string min, max;
bool minex = false, maxex = false; /* are min or max exclusive */
bool max_infinite = false; /* are max infinite */
int64_t offset = -1, count = -1;
bool removed = false, reversed = false;
- explicit CommonRangeLexSpec() = default;
+ explicit RangeLexSpec() = default;
};
-Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, RangeLexSpec *spec);
-struct CommonRangeRankSpec {
+struct RangeRankSpec {
int start, stop;
Review Comment:
Why start and stop doesn't have default value?
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843343
##########
src/commands/cmd_zset.cc:
##########
@@ -356,58 +356,41 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
-
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
-
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
-
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
-
- *output = redis::MultiBulkString(members, false);
- return Status::OK();
- } else { // range_type == kZRangeScore
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByScore(args_[1], score_spec_, &member_scores, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- return Status::OK();
+ switch (range_type_) {
+ case kZRangeLex:
+ output->append(redis::MultiBulkString(members, false));
+ return Status::OK();
+ case kZRangeAuto:
+ case kZRangeRank:
+ case kZRangeScore:
+ output->append(redis::MultiLen(member_scores.size() * (1 << with_scores_)));
Review Comment:
Make sense. I don't have strong preference here and forget why I write in this way :D
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186819646
##########
src/commands/cmd_zset.cc:
##########
@@ -356,59 +356,49 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- *output = redis::MultiBulkString(members, false);
+ if (!members.empty()) {
Review Comment:
It wouldn't reply to the request if the members and member_scores are empty lists, since the output is empty.
We still need to use MultiBulkString to reply the empty string array even if all of them are empty.
--
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 #1429: refactor: zset ranges
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186836314
##########
src/types/redis_zset.cc:
##########
@@ -220,9 +219,12 @@ rocksdb::Status ZSet::Pop(const Slice &user_key, int count, bool min, std::vecto
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
-rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const CommonRangeRankSpec &spec,
- std::vector<MemberScore> *mscores) {
- mscores->clear();
+rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const RangeRankSpec &spec, MemberScores *mscores, int *ret) {
+ if (mscores) mscores->clear();
+
+ int cnt = 0;
+ if (!ret) ret = &cnt;
Review Comment:
In this case, you will return the address of the local variable `cnt`? I'm not sure that it is safe to do that in C++. In Go - no problem :)
##########
tests/cppunit/types/zset_test.cc:
##########
@@ -325,9 +328,18 @@ TEST_F(RedisZSetTest, RemoveRangeByRank) {
}
zset_->Add(key_, ZAddFlags::Default(), &mscores, &ret);
EXPECT_EQ(fields_.size(), ret);
- zset_->RemoveRangeByRank(key_, 0, static_cast<int>(fields_.size() - 2), &ret);
+
+ RangeRankSpec spec;
+ spec.removed = true;
Review Comment:
I'm trying to think about a better name for `removed` but can't come out with something short. All I have is `with_deletion`/`need_remove`. In my opinion `removed` describes something that has been already removed, and the condition `if (removed)` we can read as `if something was removed`. However, we need something like `if (need_deletion/with_deletion/should_remove)`.
##########
src/commands/cmd_zset.cc:
##########
@@ -543,19 +528,22 @@ class CommandZRemRangeByScore : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- int size = 0;
redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- auto s = zset_db.RemoveRangeByScore(args_[1], spec_, &size);
+
+ int ret = 0;
Review Comment:
I'd prefer to have `size` or `count`/`cnt` instead of `ret` because the variable `ret` doesn't describe anything. I another functions as well.
##########
src/commands/cmd_zset.cc:
##########
@@ -356,58 +356,41 @@ class CommandZRangeGeneric : public Commander {
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
- if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByRank(args_[1], rank_spec_, &member_scores);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
-
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
-
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
-
- return Status::OK();
- } else if (range_type_ == kZRangeLex) {
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<std::string> members;
- auto s = zset_db.RangeByLex(args_[1], lex_spec_, &members, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
-
- *output = redis::MultiBulkString(members, false);
- return Status::OK();
- } else { // range_type == kZRangeScore
- int size = 0;
- redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- auto s = zset_db.RangeByScore(args_[1], score_spec_, &member_scores, &size);
- if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
- }
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
- if (!with_scores_) {
- output->append(redis::MultiLen(member_scores.size()));
- } else {
- output->append(redis::MultiLen(member_scores.size() * 2));
- }
+ std::vector<MemberScore> member_scores;
+ std::vector<std::string> members;
- for (const auto &ms : member_scores) {
- output->append(redis::BulkString(ms.member));
- if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
- }
+ rocksdb::Status s;
+ switch (range_type_) {
+ case kZRangeAuto:
+ case kZRangeRank:
+ s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeScore:
+ s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
+ break;
+ case kZRangeLex:
+ s = zset_db.RangeByLex(key_, lex_spec_, &members, nullptr);
+ break;
+ }
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
- return Status::OK();
+ switch (range_type_) {
+ case kZRangeLex:
+ output->append(redis::MultiBulkString(members, false));
+ return Status::OK();
+ case kZRangeAuto:
+ case kZRangeRank:
+ case kZRangeScore:
+ output->append(redis::MultiLen(member_scores.size() * (1 << with_scores_)));
Review Comment:
Clever trick to shift a boolean variable - one line instead of 3. But maybe just use the ternary operator `? :` here?
--
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 a diff in pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429#discussion_r1186843166
##########
src/types/redis_zset.cc:
##########
@@ -220,9 +219,12 @@ rocksdb::Status ZSet::Pop(const Slice &user_key, int count, bool min, std::vecto
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
-rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const CommonRangeRankSpec &spec,
- std::vector<MemberScore> *mscores) {
- mscores->clear();
+rocksdb::Status ZSet::RangeByRank(const Slice &user_key, const RangeRankSpec &spec, MemberScores *mscores, int *ret) {
+ if (mscores) mscores->clear();
+
+ int cnt = 0;
+ if (!ret) ret = &cnt;
Review Comment:
No. Here we change the value of local (param) variable `ret` when `nullptr` is passed. No point to local var is "returned". Just a trick to reduce `if (ret) ...` everywhere.
--
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 merged pull request #1429: refactor: zset ranges
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #1429:
URL: https://github.com/apache/incubator-kvrocks/pull/1429
--
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