[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1429: refactor: zset ranges

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