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 12:32:48 UTC
[GitHub] [incubator-kvrocks] tanruixiang opened a new pull request, #1119: Support more `ZRANGE` options.
tanruixiang opened a new pull request, #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119
This close #1117 .
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1020909949
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,119 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ by_flag_ = "BYSCORE";
+ } else if (parser.EatEqICase("BYLEX")) {
+ by_flag_ = "BYLEX";
Review Comment:
```suggestion
if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
} else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
```
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1315477714
@PragmaTwice Thanks for the analysis. I have another question, why clang can help us to solve this problem.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022799408
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ spec_ = std::make_shared<ZRangeLexSpec>();
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset = GET_OR_RET(parser.TakeInt());
+ count = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ if (by_flag_.empty()) {
+ by_flag_ = "BYINDEX";
+ spec_ = std::make_shared<ZRangeIndexSpec>();
}
- return Commander::Parse(args);
+ Status s;
+ spec_->count = count;
+ spec_->offset = offset;
+ spec_->reversed = reversed_;
+ auto &args_v = const_cast<std::vector<std::string> &>(args);
+ if (by_flag_ == "BYSCORE") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], static_cast<ZRangeSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], static_cast<ZRangeLexSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ static_cast<ZRangeIndexSpec *>(spec_.get())->min = *parse_start;
+ static_cast<ZRangeIndexSpec *>(spec_.get())->max = *parse_stop;
+ }
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
+ rocksdb::Status s;
+ int size = 0;
+ if (by_flag_ == "BYLEX") {
+ s = zset_db.RangeByLex(args_[1], *static_cast<ZRangeLexSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYSCORE") {
+ s = zset_db.RangeByScore(args_[1], *static_cast<ZRangeSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYINDEX") {
+ s = zset_db.RangeByIndex(args_[1], *static_cast<ZRangeIndexSpec *>(spec_.get()), &member_scores);
+ } else {
+ assert(false);
+ }
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
+ packageOutput(member_scores, output);
+ return Status::OK();
+ }
+
+ private:
+ void packageOutput(const std::vector<MemberScore> &member_scores, std::string *output) {
if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
} else {
- output->append(Redis::MultiLen(member_scores.size() * 2));
+ output->append(Redis::MultiLen(static_cast<int64_t>(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();
}
-
- private:
- int start_ = 0;
- int stop_ = 0;
+ std::string_view by_flag_ = "";
bool reversed_;
bool with_scores_ = false;
+ std::shared_ptr<ZrangeCommon> spec_;
};
class CommandZRevRange : public CommandZRange {
public:
CommandZRevRange() : CommandZRange(true) {}
};
-class CommandZRangeByLex : public Commander {
+class CommandZRangeByLex : public CommandZRange {
public:
- explicit CommandZRangeByLex(bool reversed = false) { spec_.reversed = reversed; }
+ explicit CommandZRangeByLex(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- Status s;
- if (spec_.reversed) {
- s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &spec_);
- } else {
- s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &spec_);
- }
- if (!s.IsOK()) {
- return Status(Status::RedisParseErr, s.Msg());
- }
- if (args.size() == 7 && Util::ToLower(args[4]) == "limit") {
- auto parse_offset = ParseInt<int>(args[5], 10);
- auto parse_count = ParseInt<int>(args[6], 10);
- if (!parse_offset || !parse_count) {
- return Status(Status::RedisParseErr, errValueNotInteger);
- }
- spec_.offset = *parse_offset;
- spec_.count = *parse_count;
- }
- return Commander::Parse(args);
+ const_cast<std::vector<std::string> &>(args).emplace_back("BYLEX");
+ if (reversed_) const_cast<std::vector<std::string> &>(args).emplace_back("REV");
+ return CommandZRange::Parse(args);
Review Comment:
It looks like I need to parse it all over again for these instructions. But I think using `const_cast` essentially splits the `zrangebylex` command into `zrange byindex`, which should make sense.
I wanted to make these commands use the same code as zrange's option as much as possible, not only to facilitate debugging, but also to reflect the fact that the commands have been deprecated.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1023009981
##########
src/types/redis_zset.h:
##########
@@ -35,46 +36,34 @@ const double kMinScore = (std::numeric_limits<float>::is_iec559 ? -std::numeric_
const double kMaxScore = (std::numeric_limits<float>::is_iec559 ? std::numeric_limits<double>::infinity()
: std::numeric_limits<double>::max());
-typedef struct ZRangeSpec {
- double min, max;
- bool minex, maxex; /* are min or max exclusive */
- int offset, count;
- bool removed, reversed;
- ZRangeSpec() {
- min = kMinScore;
- max = kMaxScore;
- minex = maxex = false;
- offset = -1;
- count = -1;
- removed = reversed = false;
- }
-} ZRangeSpec;
-
-typedef struct ZRangeLexSpec {
+struct ZrangeCommon {
Review Comment:
We have three modes, and the possibility of increasing the options in future. If we don't use inheritance, the code is simply copied and pasted in three copies. Are there any other pitfalls in using pointers? @PragmaTwice
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022777704
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ spec_ = std::make_shared<ZRangeLexSpec>();
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset = GET_OR_RET(parser.TakeInt());
+ count = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ if (by_flag_.empty()) {
+ by_flag_ = "BYINDEX";
+ spec_ = std::make_shared<ZRangeIndexSpec>();
}
- return Commander::Parse(args);
+ Status s;
+ spec_->count = count;
+ spec_->offset = offset;
+ spec_->reversed = reversed_;
+ auto &args_v = const_cast<std::vector<std::string> &>(args);
+ if (by_flag_ == "BYSCORE") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], static_cast<ZRangeSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], static_cast<ZRangeLexSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ static_cast<ZRangeIndexSpec *>(spec_.get())->min = *parse_start;
+ static_cast<ZRangeIndexSpec *>(spec_.get())->max = *parse_stop;
+ }
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
+ rocksdb::Status s;
+ int size = 0;
+ if (by_flag_ == "BYLEX") {
+ s = zset_db.RangeByLex(args_[1], *static_cast<ZRangeLexSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYSCORE") {
+ s = zset_db.RangeByScore(args_[1], *static_cast<ZRangeSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYINDEX") {
+ s = zset_db.RangeByIndex(args_[1], *static_cast<ZRangeIndexSpec *>(spec_.get()), &member_scores);
+ } else {
+ assert(false);
+ }
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
+ packageOutput(member_scores, output);
+ return Status::OK();
+ }
+
+ private:
+ void packageOutput(const std::vector<MemberScore> &member_scores, std::string *output) {
if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
} else {
- output->append(Redis::MultiLen(member_scores.size() * 2));
+ output->append(Redis::MultiLen(static_cast<int64_t>(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();
}
-
- private:
- int start_ = 0;
- int stop_ = 0;
+ std::string_view by_flag_ = "";
bool reversed_;
bool with_scores_ = false;
+ std::shared_ptr<ZrangeCommon> spec_;
};
class CommandZRevRange : public CommandZRange {
public:
CommandZRevRange() : CommandZRange(true) {}
};
-class CommandZRangeByLex : public Commander {
+class CommandZRangeByLex : public CommandZRange {
public:
- explicit CommandZRangeByLex(bool reversed = false) { spec_.reversed = reversed; }
+ explicit CommandZRangeByLex(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- Status s;
- if (spec_.reversed) {
- s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &spec_);
- } else {
- s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &spec_);
- }
- if (!s.IsOK()) {
- return Status(Status::RedisParseErr, s.Msg());
- }
- if (args.size() == 7 && Util::ToLower(args[4]) == "limit") {
- auto parse_offset = ParseInt<int>(args[5], 10);
- auto parse_count = ParseInt<int>(args[6], 10);
- if (!parse_offset || !parse_count) {
- return Status(Status::RedisParseErr, errValueNotInteger);
- }
- spec_.offset = *parse_offset;
- spec_.count = *parse_count;
- }
- return Commander::Parse(args);
+ const_cast<std::vector<std::string> &>(args).emplace_back("BYLEX");
+ if (reversed_) const_cast<std::vector<std::string> &>(args).emplace_back("REV");
+ return CommandZRange::Parse(args);
Review Comment:
I think you can find a better way to organize these code here.
To be honest, it is ugly to use `const_cast` (to break the original semantics) and takes more runtime overhead (`zrangebylex` can be more efficient to parse than `zrange`).
And more important thing is, it is not the correct way to parse `zrangebylex`. For example, it will allow some commands like `zrangebylex key x y BYLEX`.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021042971
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Oh thanks. Got it.
But I think processing same logic using different ways is an easy way to produce duplicated 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] tanruixiang commented on a diff in pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022836799
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
~~Two tasks `t1` ,` t2` can be executed concurrently, and `t2` starts allocating resources while `t1` is executing. `t2` uses the same `current_cmd_` to get allocation space while t1 is still executing, which causes t1 to use the resources held by `unique_ptr` to be freed.~~
Strange phenomenon.
Here are the failure stacks.
https://github.com/apache/incubator-kvrocks/actions/runs/3458835932/jobs/5773641529
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022836799
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
Two tasks `t1` ,` t2` can be executed concurrently, and `t2` starts allocating resources while `t1` is executing. `t2` uses the same `current_cmd_` to get allocation space while t1 is still executing, which causes t1 to use the resources held by `unique_ptr` to be freed.
Here are the failure stacks.
https://github.com/apache/incubator-kvrocks/actions/runs/3458835932/jobs/5773641529
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021042971
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Oh thanks. Got it.
But I think processing same logic using different methods is an easy way to produce duplicated 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] PragmaTwice commented on pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1315472652
I want to explain why kvrocks crashes when @tanruixiang is using `unique_ptr`.
Firstly, he creates `ZrangeCommon` and some derived structs of it, like this:
```
struct ZrangeCommon { ... };
struct ZrangeLex : ZrangeCommon { std::string x };
struct ZrangeIndex : ZrangeCommon { ... };
```
Then, he use `unique_ptr` to store `ZrangeCommon`:
```
unique_ptr<ZrangeCommon> spec_;
...
if (...) {
spec_ = make_unique<ZrangeLex>(...);
} else if (...) {
spec_ = make_unique<ZrangeIndex>(...);
}
```
Note: Unlike `shared_ptr`, `unique_ptr` does not do type erasure, which means that the default deleter in `unique_ptr` is determined at compile time for the destructor. In this case, destructor of `ZrangeCommon` will be called when `spec_` is destructed. Note that the actual object type at this point may be `ZRangeLex`, and calling the wrong destructor may not only lead to resource leaks, but also to other catastrophic consequences due to UB.
The design of `shared_ptr` is different in that `shared_ptr` does type erasure and its deleter is stored in runtime, which makes it store the exact function pointer of the constructore at construction (which, of course, adds runtime overhead), so there is no such problem.
So a better solution is to use a virtual destructor: `shared_ptr` is not needed here, because `shared_ptr` has extra overhead and usefulness:
```
struct ZrangeCommon {
...
virtual ~ZrangeCommon() {}
};
```
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1023001395
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
I think union may have similar problem: we need to determine the correct dtor since there are some non-POD in spec_ (i.e. std::string in LexSpec), maybe we can just use multiple values or std::variant 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] torwig commented on a diff in pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1020940458
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
}
- return Commander::Parse(args);
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
- if (!s.ok()) {
- return Status(Status::RedisExecErr, s.ToString());
- }
- if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ if (by_flag_ == "BYLEX") {
+ int size = 0;
+ std::vector<std::string> members;
+ rocksdb::Status s = zset_db.RangeByLex(args_[1], specLex_, &members, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::MultiBulkString(members, false);
+ } else if (by_flag_ == "BYSCORE") {
+ int size = 0;
+ std::vector<MemberScore> member_scores;
+ rocksdb::Status s = zset_db.RangeByScore(args_[1], spec_, &member_scores, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
+ } else if (by_flag_ == "BYINDEX") {
+ std::vector<MemberScore> member_scores;
+ uint8_t flags = !reversed_ ? 0 : kZSetReversed;
+ rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores, offset_, count_);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
} 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)));
+ assert(false);
}
return Status::OK();
}
private:
+ std::string_view by_flag_ = "";
+ int offset_ = 0;
+ int count_ = -1;
int start_ = 0;
int stop_ = 0;
- bool reversed_;
+ bool reversed_ = false;
bool with_scores_ = false;
+ ZRangeLexSpec specLex_;
Review Comment:
Change camel case: `specLex_` => `spec_lex_`
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021070659
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Here it is just to use the previous interface. I can use another wrapper interface to solve this problem.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022982825
##########
src/types/redis_zset.h:
##########
@@ -35,46 +36,34 @@ const double kMinScore = (std::numeric_limits<float>::is_iec559 ? -std::numeric_
const double kMaxScore = (std::numeric_limits<float>::is_iec559 ? std::numeric_limits<double>::infinity()
: std::numeric_limits<double>::max());
-typedef struct ZRangeSpec {
- double min, max;
- bool minex, maxex; /* are min or max exclusive */
- int offset, count;
- bool removed, reversed;
- ZRangeSpec() {
- min = kMinScore;
- max = kMaxScore;
- minex = maxex = false;
- offset = -1;
- count = -1;
- removed = reversed = false;
- }
-} ZRangeSpec;
-
-typedef struct ZRangeLexSpec {
+struct ZrangeCommon {
Review Comment:
@tanruixiang I prefer the old way to separate `ZRangeSpec` and `ZRangeLexSpec`, it's more clear when parsing even with a bit of duplicate code and we also don't need to use the ptr in the Parse function.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022795738
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
Concurrent instructions result in destructions on line `auto s = svr_->LookupAndCreateCommand(cmd_tokens.front(), ¤t_cmd_);`.
```
void Connection::ExecuteCommands(std::deque<CommandTokens> *to_process_cmds) {
Config *config = svr_->GetConfig();
std::string reply, password = config->requirepass;
while (!to_process_cmds->empty()) {
auto cmd_tokens = to_process_cmds->front();
to_process_cmds->pop_front();
if (IsFlagEnabled(Redis::Connection::kCloseAfterReply) && !IsFlagEnabled(Connection::kMultiExec)) break;
auto s = svr_->LookupAndCreateCommand(cmd_tokens.front(), ¤t_cmd_);
```
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022912265
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
It is not correct. If `CommandZrange` has this problem, then other commands will also have this problem.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021070659
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Here it is just to use the previous interface.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022781793
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
Could you explain a bit about the use of `shared_ptr` 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] tanruixiang commented on a diff in pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021041473
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Here it is straightforward to use `reversed_`. It is handled in the following `execute`.
```
uint8_t flags = !reversed_ ? 0 : kZSetReversed;
```
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021042116
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
}
- return Commander::Parse(args);
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
- if (!s.ok()) {
- return Status(Status::RedisExecErr, s.ToString());
- }
- if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ if (by_flag_ == "BYLEX") {
+ int size = 0;
+ std::vector<std::string> members;
+ rocksdb::Status s = zset_db.RangeByLex(args_[1], specLex_, &members, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::MultiBulkString(members, false);
+ } else if (by_flag_ == "BYSCORE") {
+ int size = 0;
+ std::vector<MemberScore> member_scores;
+ rocksdb::Status s = zset_db.RangeByScore(args_[1], spec_, &member_scores, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
+ } else if (by_flag_ == "BYINDEX") {
+ std::vector<MemberScore> member_scores;
+ uint8_t flags = !reversed_ ? 0 : kZSetReversed;
+ rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores, offset_, count_);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
Review Comment:
When writing code, it is important to prevent duplicate code fragments.
Instead of mindlessly coding, I think it is better to think more about composition and abstraction.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022836799
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
> I still can't understand this point, the lookup command is unique ptr and it should not have the race between workers.
There are two tasks `t1`and `t2` at the same time. `t2` uses the same `current_cmd_` to get allocation space while t1 is still executing, which causes t1 to use the resources held by `unique_ptr` to be freed.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022836799
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
Two tasks `t1` ,` t2` can be executed concurrently, and `t2` starts allocating resources while `t1` is executing. `t2` uses the same `current_cmd_` to get allocation space while t1 is still executing, which causes t1 to use the resources held by `unique_ptr` to be freed.
--
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 closed pull request #1119: Support more `ZRANGE` options
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun closed pull request #1119: Support more `ZRANGE` options
URL: https://github.com/apache/incubator-kvrocks/pull/1119
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1328010014
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] tanruixiang commented on pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1328010660
I will try to finish this pr as soon as possible before the big refactoring.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021042116
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
}
- return Commander::Parse(args);
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
- if (!s.ok()) {
- return Status(Status::RedisExecErr, s.ToString());
- }
- if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ if (by_flag_ == "BYLEX") {
+ int size = 0;
+ std::vector<std::string> members;
+ rocksdb::Status s = zset_db.RangeByLex(args_[1], specLex_, &members, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ *output = Redis::MultiBulkString(members, false);
+ } else if (by_flag_ == "BYSCORE") {
+ int size = 0;
+ std::vector<MemberScore> member_scores;
+ rocksdb::Status s = zset_db.RangeByScore(args_[1], spec_, &member_scores, &size);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
+ } else if (by_flag_ == "BYINDEX") {
+ std::vector<MemberScore> member_scores;
+ uint8_t flags = !reversed_ ? 0 : kZSetReversed;
+ rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores, offset_, count_);
+ if (!s.ok()) {
+ return Status(Status::RedisExecErr, s.ToString());
+ }
+ if (!with_scores_) {
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
+ } else {
+ output->append(Redis::MultiLen(static_cast<int64_t>(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)));
+ }
Review Comment:
When writing code, it is important to prevent duplicate code fragments.
Instead of mindlessly coding, think about composition and abstraction.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022777704
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ spec_ = std::make_shared<ZRangeLexSpec>();
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset = GET_OR_RET(parser.TakeInt());
+ count = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ if (by_flag_.empty()) {
+ by_flag_ = "BYINDEX";
+ spec_ = std::make_shared<ZRangeIndexSpec>();
}
- return Commander::Parse(args);
+ Status s;
+ spec_->count = count;
+ spec_->offset = offset;
+ spec_->reversed = reversed_;
+ auto &args_v = const_cast<std::vector<std::string> &>(args);
+ if (by_flag_ == "BYSCORE") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], static_cast<ZRangeSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ if (spec_->reversed) std::swap(args_v[2], args_v[3]);
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], static_cast<ZRangeLexSpec *>(spec_.get()));
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ static_cast<ZRangeIndexSpec *>(spec_.get())->min = *parse_start;
+ static_cast<ZRangeIndexSpec *>(spec_.get())->max = *parse_stop;
+ }
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
std::vector<MemberScore> member_scores;
- uint8_t flags = !reversed_ ? 0 : kZSetReversed;
- rocksdb::Status s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
+ rocksdb::Status s;
+ int size = 0;
+ if (by_flag_ == "BYLEX") {
+ s = zset_db.RangeByLex(args_[1], *static_cast<ZRangeLexSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYSCORE") {
+ s = zset_db.RangeByScore(args_[1], *static_cast<ZRangeSpec *>(spec_.get()), &member_scores, &size);
+ } else if (by_flag_ == "BYINDEX") {
+ s = zset_db.RangeByIndex(args_[1], *static_cast<ZRangeIndexSpec *>(spec_.get()), &member_scores);
+ } else {
+ assert(false);
+ }
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
+ packageOutput(member_scores, output);
+ return Status::OK();
+ }
+
+ private:
+ void packageOutput(const std::vector<MemberScore> &member_scores, std::string *output) {
if (!with_scores_) {
- output->append(Redis::MultiLen(member_scores.size()));
+ output->append(Redis::MultiLen(static_cast<int64_t>(member_scores.size())));
} else {
- output->append(Redis::MultiLen(member_scores.size() * 2));
+ output->append(Redis::MultiLen(static_cast<int64_t>(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();
}
-
- private:
- int start_ = 0;
- int stop_ = 0;
+ std::string_view by_flag_ = "";
bool reversed_;
bool with_scores_ = false;
+ std::shared_ptr<ZrangeCommon> spec_;
};
class CommandZRevRange : public CommandZRange {
public:
CommandZRevRange() : CommandZRange(true) {}
};
-class CommandZRangeByLex : public Commander {
+class CommandZRangeByLex : public CommandZRange {
public:
- explicit CommandZRangeByLex(bool reversed = false) { spec_.reversed = reversed; }
+ explicit CommandZRangeByLex(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- Status s;
- if (spec_.reversed) {
- s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &spec_);
- } else {
- s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &spec_);
- }
- if (!s.IsOK()) {
- return Status(Status::RedisParseErr, s.Msg());
- }
- if (args.size() == 7 && Util::ToLower(args[4]) == "limit") {
- auto parse_offset = ParseInt<int>(args[5], 10);
- auto parse_count = ParseInt<int>(args[6], 10);
- if (!parse_offset || !parse_count) {
- return Status(Status::RedisParseErr, errValueNotInteger);
- }
- spec_.offset = *parse_offset;
- spec_.count = *parse_count;
- }
- return Commander::Parse(args);
+ const_cast<std::vector<std::string> &>(args).emplace_back("BYLEX");
+ if (reversed_) const_cast<std::vector<std::string> &>(args).emplace_back("REV");
+ return CommandZRange::Parse(args);
Review Comment:
I think you can find a better way to organize these code here.
To be honest, it is ugly to use `const_cast` (to break the original semantics) and takes more runtime overhead (`zrangebylex` can be more efficient to parse than `zrange`).
And more important is, it is not the correct way to parse `zrangebylex`. For example, it will allow some commands like `zrangebylex key x y BYLEX`.
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022992855
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
I think it's not necessary to generalize the `sepc_` if it makes the code not easy to read. We even can have the union of byLexSpec and byIndexSpec, which is easier to understand than using share_ptr 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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1023001395
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
I think union may have similar problem: we need to determine the correct dtor since there are some non-POD in spec_ (i.e. std::string in LexSpec), maybe we can just use multiple value or std::variant 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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1021040224
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,44 +2555,117 @@ class CommandZRange : public Commander {
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ } else if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- start_ = *parse_start;
- stop_ = *parse_stop;
- if (args.size() > 4 && (Util::ToLower(args[4]) == "withscores")) {
- with_scores_ = true;
+ Status s;
+ if (by_flag_ == "BYSCORE") {
+ spec_.count = count_;
+ spec_.offset = offset_;
+ spec_.reversed = reversed_;
+ if (spec_.reversed) {
+ s = Redis::ZSet::ParseRangeSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::ZSet::ParseRangeSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else if (by_flag_ == "BYLEX") {
+ specLex_.count = count_;
+ specLex_.offset = offset_;
+ specLex_.reversed = reversed_;
+ if (specLex_.reversed) {
+ s = Redis::ZSet::ParseRangeLexSpec(args[3], args[2], &specLex_);
+ } else {
+ s = Redis::ZSet::ParseRangeLexSpec(args[2], args[3], &specLex_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
+ auto parse_start = ParseInt<int>(args[2], 10);
+ auto parse_stop = ParseInt<int>(args[3], 10);
+ if (!parse_start || !parse_stop) {
+ return Status(Status::RedisParseErr, errValueNotInteger);
+ }
+ start_ = *parse_start;
+ stop_ = *parse_stop;
Review Comment:
Forget to handle `REV` 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] git-hulk commented on a diff in pull request #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1022831750
##########
src/commands/redis_cmd.cc:
##########
@@ -2554,152 +2556,135 @@ class CommandZPopMax : public CommandZPop {
class CommandZRange : public Commander {
public:
explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
-
Status Parse(const std::vector<std::string> &args) override {
- auto parse_start = ParseInt<int>(args[2], 10);
- auto parse_stop = ParseInt<int>(args[3], 10);
- if (!parse_start || !parse_stop) {
- return Status(Status::RedisParseErr, errValueNotInteger);
+ int offset = 0;
+ int count = -1;
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYSCORE", by_flag_)) {
+ spec_ = std::make_shared<ZRangeSpec>();
Review Comment:
I still can't understand this point, the lookup command is unique ptr and it should not have the race between workers.
##########
src/types/redis_zset.cc:
##########
@@ -224,7 +224,10 @@ rocksdb::Status ZSet::Pop(const Slice &user_key, int count, bool min, std::vecto
}
rocksdb::Status ZSet::Range(const Slice &user_key, int start, int stop, uint8_t flags,
- std::vector<MemberScore> *mscores) {
+ std::vector<MemberScore> *mscores, int limit_offset, int limit_count) {
Review Comment:
Can simplify the naming: `limit_offset` => `offset`, same as the `limit_count`
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#discussion_r1023009981
##########
src/types/redis_zset.h:
##########
@@ -35,46 +36,34 @@ const double kMinScore = (std::numeric_limits<float>::is_iec559 ? -std::numeric_
const double kMaxScore = (std::numeric_limits<float>::is_iec559 ? std::numeric_limits<double>::infinity()
: std::numeric_limits<double>::max());
-typedef struct ZRangeSpec {
- double min, max;
- bool minex, maxex; /* are min or max exclusive */
- int offset, count;
- bool removed, reversed;
- ZRangeSpec() {
- min = kMinScore;
- max = kMaxScore;
- minex = maxex = false;
- offset = -1;
- count = -1;
- removed = reversed = false;
- }
-} ZRangeSpec;
-
-typedef struct ZRangeLexSpec {
+struct ZrangeCommon {
Review Comment:
We have three modes, and the possibility of increasing the options int future. If we don't use inheritance, the code is simply copied and pasted in three copies. Are there any other pitfalls in using pointers? @PragmaTwice
--
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 #1119: Support more `ZRANGE` options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1315488008
> @PragmaTwice Thanks for the analysis. I have another question, why clang can help us to solve this problem.
Calling wrong dtor is an UB, and UB can cause anything.
--
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 #1119: Support more `ZRANGE` options
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1119:
URL: https://github.com/apache/incubator-kvrocks/pull/1119#issuecomment-1537129950
Closing as conflicts...
Any progress requires rework 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