[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1428: feat: Catch up `ZRANGE` options

["tisonkun (via GitHub)" <gi...@apache.org>]

tisonkun commented on code in PR #1428:
URL: https://github.com/apache/incubator-kvrocks/pull/1428#discussion_r1186792848


##########
src/commands/cmd_zset.cc:
##########
@@ -265,170 +266,190 @@ class CommandZPopMax : public CommandZPop {
   CommandZPopMax() : CommandZPop(false) {}
 };
 
-class CommandZRange : public Commander {
+class CommandZRangeGeneric : public Commander {
  public:
-  explicit CommandZRange(bool reversed = false) : reversed_(reversed) {}
+  explicit CommandZRangeGeneric(ZRangeType range_type = kZRangeAuto, ZRangeDirection direction = kZRangeDirectionAuto)
+      : range_type_(range_type), direction_(direction) {}
 
   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::RedisParseErr, errValueNotInteger};
+    key_ = args[1];
+
+    int64_t offset = 0;
+    int64_t count = -1;
+    // skip the <CMD> <src> <min> <max> args and parse remaining optional arguments
+    CommandParser parser(args, 4);
+    while (parser.Good()) {
+      if (parser.EatEqICase("withscores")) {
+        with_scores_ = true;
+      } else if (parser.EatEqICase("limit")) {
+        auto parse_offset = parser.TakeInt<int64_t>();
+        auto parse_count = parser.TakeInt<int64_t>();
+        if (!parse_offset || !parse_count) {
+          return {Status::RedisParseErr, errValueNotInteger};
+        }
+        offset = *parse_offset;
+        count = *parse_count;
+      } else if (range_type_ == kZRangeAuto && parser.EatEqICase("bylex")) {
+        range_type_ = kZRangeLex;
+      } else if (range_type_ == kZRangeAuto && parser.EatEqICase("byscore")) {
+        range_type_ = kZRangeScore;
+      } else if (direction_ == kZRangeDirectionAuto && parser.EatEqICase("rev")) {
+        direction_ = kZRangeDirectionReverse;
+      } else {
+        return parser.InvalidSyntax();
+      }
     }
 
-    start_ = *parse_start;
-    stop_ = *parse_stop;
-    if (args.size() > 4 && (util::ToLower(args[4]) == "withscores")) {
-      with_scores_ = true;
+    // use defaults if not overridden by arguments
+    if (range_type_ == kZRangeAuto) {
+      range_type_ = kZRangeRank;
+    }
+    if (direction_ == kZRangeDirectionAuto) {
+      direction_ = kZRangeDirectionForward;
     }
 
-    return Commander::Parse(args);
-  }
-
-  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;
-    auto s = zset_db.Range(args_[1], start_, stop_, flags, &member_scores);
-    if (!s.ok()) {
-      return {Status::RedisExecErr, s.ToString()};
+    // check for conflicting arguments
+    if (with_scores_ && range_type_ == kZRangeLex) {
+      return {Status::RedisParseErr, "syntax error, WITHSCORES not supported in combination with BYLEX"};
+    }
+    if (count != -1 && range_type_ == kZRangeRank) {
+      return {Status::RedisParseErr,
+              "syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX"};
     }
 
-    if (!with_scores_) {
-      output->append(redis::MultiLen(member_scores.size()));
-    } else {
-      output->append(redis::MultiLen(member_scores.size() * 2));
+    // resolve index of <min> <max>
+    int min_idx = 2;
+    int max_idx = 3;
+    if (direction_ == kZRangeDirectionReverse && (range_type_ == kZRangeLex || range_type_ == kZRangeScore)) {
+      min_idx = 3;
+      max_idx = 2;
     }
 
-    for (const auto &ms : member_scores) {
-      output->append(redis::BulkString(ms.member));
-      if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
+    // parse range spec
+    switch (range_type_) {
+      case kZRangeAuto:
+      case kZRangeRank:
+        GET_OR_RET(ParseRangeRankSpec(args[min_idx], args[max_idx], &rank_spec_));
+        if (direction_ == kZRangeDirectionReverse) {
+          rank_spec_.reversed = true;
+        }
+        break;
+      case kZRangeLex:
+        GET_OR_RET(ParseRangeLexSpec(args[min_idx], args[max_idx], &lex_spec_));
+        lex_spec_.offset = offset;
+        lex_spec_.count = count;
+        if (direction_ == kZRangeDirectionReverse) {
+          lex_spec_.reversed = true;
+        }
+        break;
+      case kZRangeScore:
+        GET_OR_RET(ParseRangeScoreSpec(args[min_idx], args[max_idx], &score_spec_));
+        score_spec_.offset = offset;
+        score_spec_.count = count;
+        if (direction_ == kZRangeDirectionReverse) {
+          score_spec_.reversed = true;
+        }
+        break;
     }
 
     return Status::OK();
   }
 
- private:
-  int start_ = 0;
-  int stop_ = 0;
-  bool reversed_;
-  bool with_scores_ = false;
-};
+  Status Execute(Server *svr, Connection *conn, std::string *output) override {
+    if (range_type_ == kZRangeAuto || range_type_ == kZRangeRank) {

Review Comment:
   This staff can be later refactor to dedup. But I keep it as is so that we don't introduce too much unstable in one PR



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org