[GitHub] [incubator-kvrocks] manchurio commented on a diff in pull request #1022: Implement the ZADD options

[GitBox <gi...@apache.org>]

manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1001911867


##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
 
   Status Execute(Server *svr, Connection *conn, std::string *output) override {
     int ret;
+    double old_score = member_scores_[0].score;
+    bool incr = (flags_ & kZSetIncr) != 0;
     Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
-    rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+    rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
     if (!s.ok()) {
       return Status(Status::RedisExecErr, s.ToString());
     }
-    *output = Redis::Integer(ret);
+    if (incr) {
+      auto new_score = member_scores_[0].score;
+      bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+      if ((nx || xx || lt || gt) && old_score == new_score &&
+          ret == 0) {  // not the first time using incr && score not changed
+        *output = Redis::NilString();
+        return Status::OK();
+      }
+      *output = Redis::BulkString(Util::Float2String(new_score));
+    } else {
+      *output = Redis::Integer(ret);
+    }
     return Status::OK();
   }
 
  private:
   std::vector<MemberScore> member_scores_;
-};
+  uint8_t flags_ = 0;
+
+  void parseOptions(const std::vector<std::string> &args, unsigned &index);
+  Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+  std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+                                                        {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+  constexpr unsigned max_index = 6;
+  for (unsigned i = 2; i < max_index; i++) {
+    auto option = Util::ToLower(args[i]);
+    auto it = options.find(option);
+    if (it != options.end()) {
+      flags_ |= it->second;
+      index++;
+    } else {
+      break;
+    }

Review Comment:
   Not only determine whether the key exists, but also get the associated value. And in your code, it will find value twice, bad for performance.



-- 
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