From ffa097fa9acef4576ffde18a61790d9d4cd93996 Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 20 Oct 2022 18:47:26 +0800 Subject: [PATCH 01/27] Add zadd options in headers --- src/types/redis_zset.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/types/redis_zset.h b/src/types/redis_zset.h index 42b3fd5b50c..f1952e43434 100644 --- a/src/types/redis_zset.h +++ b/src/types/redis_zset.h @@ -82,6 +82,9 @@ enum ZSetFlags { kZSetXX = 1 << 2, kZSetReversed = 1 << 3, kZSetRemoved = 1 << 4, + kZSetGT = 1 << 5, + kZSetLT = 1 << 6, + kZSetCH = 1 << 7, }; namespace Redis { From f4f3ebbc78e8ca6b27ceebcc9fecb2b6ced42a6f Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 20 Oct 2022 20:44:35 +0800 Subject: [PATCH 02/27] Parse options for zadd. --- src/commands/redis_cmd.cc | 42 +++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 7bc0b0a8373..d32c2a933ee 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2364,13 +2364,14 @@ class CommandSInterStore : public Commander { class CommandZAdd : public Commander { public: - Status Parse(const std::vector &args) override { - if (args.size() % 2 != 0) { + Status Parse(const std::vector &args) override { + unsigned index = 2; + parseOptions(args, index); + if ((args.size()-index) % 2 != 0) { return Status(Status::RedisParseErr, errInvalidSyntax); } - try { - for (unsigned i = 2; i < args.size(); i += 2) { + for (unsigned i = index; i < args.size(); i += 2) { double score = std::stod(args[i]); if (std::isnan(score)) { return Status(Status::RedisParseErr, "ERR score is not a valid float"); @@ -2386,7 +2387,7 @@ class CommandZAdd : public Commander { Status Execute(Server *svr, Connection *conn, std::string *output) override { int ret; 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()); } @@ -2396,7 +2397,36 @@ class CommandZAdd : public Commander { private: std::vector member_scores_; -}; + uint8_t flags = 0; + + void parseOptions(const std::vector &args, unsigned& index); + rocksdb::Status validateFlags(); +}; + +void CommandZAdd::parseOptions(const std::vector &args, unsigned& index) { + constexpr unsigned max_options = 4; + for (unsigned i = 2; i < max_options; i++) { + auto option = args[i].c_str(); + if (!strcasecmp(option, "xx")) { + flags |= kZSetXX; + index++; + } else if (!strcasecmp(option, "nx")) { + flags |= kZSetNX; + index++; + } else if (!strcasecmp(option, "incr")) { + flags |= kZSetIncr; + index++; + } else if (!strcasecmp(option, "ch")) { + flags |= kZSetCH; + index++; + } else if (!strcasecmp(option, "gt")) { + flags |= kZSetGT; + } else if (!strcasecmp(option, "lt")) { + flags |= kZSetLT; + index++; + } + } +} class CommandZCount : public Commander { public: From 8ce109350bfb0ea1f50b01ef4727396921b6d716 Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 20 Oct 2022 20:52:34 +0800 Subject: [PATCH 03/27] Validate flags in zadd. --- src/commands/redis_cmd.cc | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index d32c2a933ee..756fdd6f954 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2367,6 +2367,7 @@ class CommandZAdd : public Commander { Status Parse(const std::vector &args) override { unsigned index = 2; parseOptions(args, index); + rocksdb::Status s = validateFlags(); if ((args.size()-index) % 2 != 0) { return Status(Status::RedisParseErr, errInvalidSyntax); } @@ -2400,7 +2401,7 @@ class CommandZAdd : public Commander { uint8_t flags = 0; void parseOptions(const std::vector &args, unsigned& index); - rocksdb::Status validateFlags(); + Status validateFlags(); }; void CommandZAdd::parseOptions(const std::vector &args, unsigned& index) { @@ -2428,6 +2429,30 @@ void CommandZAdd::parseOptions(const std::vector &args, unsigned& i } } +Status CommandZAdd::validateFlags() { + if (!flags) { + return {}; + } + bool nx = (flags & kZSetNX) != 0; + bool xx = (flags & kZSetXX) != 0; + bool lt = (flags & kZSetLT) != 0; + bool gt = (flags & kZSetGT) != 0; + bool ch = (flags & kZSetCH) != 0; + if (nx && xx) { + return Status(Status::RedisParseErr, errInvalidSyntax); + } + if (lt && gt) { + return Status(Status::RedisParseErr, errInvalidSyntax); + } + if (lt && nx) { + return Status(Status::RedisParseErr, errInvalidSyntax); + } + if (gt && nx) { + return Status(Status::RedisParseErr, errInvalidSyntax); + } + return {}; +} + class CommandZCount : public Commander { public: Status Parse(const std::vector &args) override { From 09bf9e75a7dc8382c4725c530adf9ba2b0139f58 Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 11:38:22 +0800 Subject: [PATCH 04/27] Refactor parseOptions. --- src/commands/redis_cmd.cc | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 756fdd6f954..1f2a203b7da 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2367,7 +2367,10 @@ class CommandZAdd : public Commander { Status Parse(const std::vector &args) override { unsigned index = 2; parseOptions(args, index); - rocksdb::Status s = validateFlags(); + Status s = validateFlags(); + if (!s.IsOK()) { + return s; + } if ((args.size()-index) % 2 != 0) { return Status(Status::RedisParseErr, errInvalidSyntax); } @@ -2388,7 +2391,7 @@ class CommandZAdd : public Commander { Status Execute(Server *svr, Connection *conn, std::string *output) override { int ret; Redis::ZSet zset_db(svr->storage_, conn->GetNamespace()); - rocksdb::Status s = zset_db.Add(args_[1], flags, &member_scores_, &ret); + rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret); if (!s.ok()) { return Status(Status::RedisExecErr, s.ToString()); } @@ -2398,33 +2401,29 @@ class CommandZAdd : public Commander { private: std::vector member_scores_; - uint8_t flags = 0; + uint8_t flags_ = 0; void parseOptions(const std::vector &args, unsigned& index); Status validateFlags(); }; void CommandZAdd::parseOptions(const std::vector &args, unsigned& index) { + std::unordered_map options = { + {"xx", kZSetXX}, + {"nx", kZSetNX}, + {"ch", kZSetCH}, + {"lt", kZSetLT}, + {"gt", kZSetGT}, + {"incr", kZSetIncr}, + }; constexpr unsigned max_options = 4; for (unsigned i = 2; i < max_options; i++) { - auto option = args[i].c_str(); - if (!strcasecmp(option, "xx")) { - flags |= kZSetXX; - index++; - } else if (!strcasecmp(option, "nx")) { - flags |= kZSetNX; - index++; - } else if (!strcasecmp(option, "incr")) { - flags |= kZSetIncr; - index++; - } else if (!strcasecmp(option, "ch")) { - flags |= kZSetCH; - index++; - } else if (!strcasecmp(option, "gt")) { - flags |= kZSetGT; - } else if (!strcasecmp(option, "lt")) { - flags |= kZSetLT; + auto option = Util::ToLower(args[i]); + if (auto it = options.find(option); it != options.end()) { + flags_ |= it->second; index++; + } else { + break; } } } From 0baa9b71afb790ac65a13c778e3711e46648b39c Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 17:53:40 +0800 Subject: [PATCH 05/27] Handle zadd options --- src/types/redis_zset.cc | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index fca4b2e3260..8adbaf2eb8e 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -49,6 +49,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector 0) { + if (flags & kZSetNX) { + continue; + } std::string old_score_bytes; s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes); if (!s.ok() && !s.IsNotFound()) return s; if (s.ok()) { double old_score = DecodeDouble(old_score_bytes.data()); - if (flags == kZSetIncr) { + if (flags & kZSetIncr) { + if (lt && (*mscores)[i].score >= 0) { + continue; + } else if (gt && (*mscores)[i].score <= 0) { + continue; + } (*mscores)[i].score += old_score; if (std::isnan((*mscores)[i].score)) { return rocksdb::Status::InvalidArgument("resulting score is not a number (NaN)"); } } if ((*mscores)[i].score != old_score) { + if (lt && (*mscores)[i].score >= old_score ) { + continue; + } else if (gt && (*mscores)[i].score <= old_score) { + continue; + } old_score_bytes.append((*mscores)[i].member); std::string old_score_key; InternalKey(ns_key, old_score_bytes, metadata.version, storage_->IsSlotIdEncoded()).Encode(&old_score_key); @@ -94,10 +111,14 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vectorIsSlotIdEncoded()).Encode(&new_score_key); batch.Put(score_cf_handle_, new_score_key, Slice()); + changed++; } continue; } } + if (flags & kZSetXX) { + continue; + } std::string score_bytes, score_key; PutDouble(&score_bytes, (*mscores)[i].score); batch.Put(member_key, score_bytes); @@ -113,6 +134,9 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vectorWrite(storage_->DefaultWriteOptions(), &batch); } From 0e5158200063175afb8e680bc8dd4be4d842a841 Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 17:53:59 +0800 Subject: [PATCH 06/27] Handel zadd response with options --- src/commands/redis_cmd.cc | 48 +++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 1f2a203b7da..cd7299db4a0 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -73,6 +73,7 @@ const char *errUnbalancedStreamList = "Unbalanced XREAD list of streams: for each stream key an ID or '$' must be specified."; const char *errTimeoutIsNegative = "timeout is negative"; const char *errLimitOptionNotAllowed = "syntax error, LIMIT cannot be used without the special ~ option"; +const char *errZSetLTGTNX = "GT, LT, and/or NX options at the same time are not compatible"; enum class AuthResult { OK, @@ -2367,12 +2368,14 @@ class CommandZAdd : public Commander { Status Parse(const std::vector &args) override { unsigned index = 2; parseOptions(args, index); - Status s = validateFlags(); - if (!s.IsOK()) { + if (auto s = validateFlags(); !s.IsOK()) { return s; } - if ((args.size()-index) % 2 != 0) { - return Status(Status::RedisParseErr, errInvalidSyntax); + if (auto left = (args.size()-index); left > 0) { + if ((flags_ & kZSetIncr) && left != 2) { + return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair"); + } else if (left % 2 != 0) + return Status(Status::RedisParseErr, errInvalidSyntax); } try { for (unsigned i = index; i < args.size(); i += 2) { @@ -2390,12 +2393,24 @@ 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], 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(); } @@ -2416,8 +2431,8 @@ void CommandZAdd::parseOptions(const std::vector &args, unsigned& i {"gt", kZSetGT}, {"incr", kZSetIncr}, }; - constexpr unsigned max_options = 4; - for (unsigned i = 2; i < max_options; i++) { + constexpr unsigned max_index = 6; + for (unsigned i = 2; i < max_index; i++) { auto option = Util::ToLower(args[i]); if (auto it = options.find(option); it != options.end()) { flags_ |= it->second; @@ -2429,25 +2444,24 @@ void CommandZAdd::parseOptions(const std::vector &args, unsigned& i } Status CommandZAdd::validateFlags() { - if (!flags) { + if (!flags_) { return {}; } - bool nx = (flags & kZSetNX) != 0; - bool xx = (flags & kZSetXX) != 0; - bool lt = (flags & kZSetLT) != 0; - bool gt = (flags & kZSetGT) != 0; - bool ch = (flags & kZSetCH) != 0; + bool nx = (flags_ & kZSetNX) != 0; + bool xx = (flags_ & kZSetXX) != 0; + bool lt = (flags_ & kZSetLT) != 0; + bool gt = (flags_ & kZSetGT) != 0; if (nx && xx) { - return Status(Status::RedisParseErr, errInvalidSyntax); + return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible"); } if (lt && gt) { - return Status(Status::RedisParseErr, errInvalidSyntax); + return Status(Status::RedisParseErr, errZSetLTGTNX); } if (lt && nx) { - return Status(Status::RedisParseErr, errInvalidSyntax); + return Status(Status::RedisParseErr, errZSetLTGTNX); } if (gt && nx) { - return Status(Status::RedisParseErr, errInvalidSyntax); + return Status(Status::RedisParseErr, errZSetLTGTNX); } return {}; } From 3af1e0dd96ad5f1fdcd8569642d1913a0e4e7a91 Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 17:54:17 +0800 Subject: [PATCH 07/27] Zadd options unit test. --- tests/gocase/unit/type/zset/zset_test.go | 66 ++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index 616387e1037..4c0ae58773f 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -93,6 +93,72 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s require.Equal(t, float64(30), rdb.ZScore(ctx, "ztmp", "x").Val()) }) + t.Run(fmt.Sprintf("ZSET ZADD INCR option supports a single pair - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Contains(t, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}, {Member: "adc"}}}).Err(), + "INCR option supports a single increment-element pair") + }) + t.Run(fmt.Sprintf("ZSET ZADD IncrMixedOtherOptions - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + + rdb.Del(ctx, "ztmp") + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{XX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + + rdb.Del(ctx, "ztmp") + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 3.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Err()) + + rdb.Del(ctx, "ztmp") + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + }) + + t.Run(fmt.Sprintf("ZSET ZADD LT/GT with other options - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 100}}}).Val()) + require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "lt", "gt", "1", "m1", "2", "m2").Err(), + "GT, LT, and/or NX options at the same time are not compatible") + + rdb.Del(ctx, "ztmp") + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.2}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 0.5}}}).Val()) + + rdb.Del(ctx, "newAbc1", "newAbc2") + require.Equal(t, int64(2), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Ch: true, Members: []redis.Z{{Member: "abc", Score: 0.5}, {Member: "newAbc1", Score: 10}, {Member: "newAbc2"}}}).Val()) + }) + + t.Run(fmt.Sprintf("ZSET ZADD NX/XX option supports a single pair - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "nx", "xx", "1", "m1", "2", "m2").Err(), + "XX and NX options at the same time are not compatible") + + require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "lt", "nx", "1", "m1", "2", "m2").Err(), + "GT, LT, and/or NX options at the same time are not compatible") + require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "gt", "nx", "1", "m1", "2", "m2").Err(), + "GT, LT, and/or NX options at the same time are not compatible") + + rdb.Del(ctx, "ztmp") + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + }) + t.Run(fmt.Sprintf("ZSET element can't be set to NaN with ZADD - %s", encoding), func(t *testing.T) { require.Contains(t, rdb.ZAdd(ctx, "myzset", redis.Z{Score: math.NaN(), Member: "abc"}).Err(), "float") }) From 9ab913c338d5fab29ff0aff7978b6b345a22831f Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 18:56:20 +0800 Subject: [PATCH 08/27] Fix problems about clang-format. --- src/commands/redis_cmd.cc | 21 ++++++++------------- src/types/redis_zset.cc | 2 +- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index cd7299db4a0..71b0dd287f3 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2371,7 +2371,7 @@ class CommandZAdd : public Commander { if (auto s = validateFlags(); !s.IsOK()) { return s; } - if (auto left = (args.size()-index); left > 0) { + if (auto left = (args.size() - index); left > 0) { if ((flags_ & kZSetIncr) && left != 2) { return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair"); } else if (left % 2 != 0) @@ -2402,8 +2402,9 @@ class CommandZAdd : public Commander { } 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 + 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(); } @@ -2418,19 +2419,13 @@ class CommandZAdd : public Commander { std::vector member_scores_; uint8_t flags_ = 0; - void parseOptions(const std::vector &args, unsigned& index); + void parseOptions(const std::vector &args, unsigned &index); Status validateFlags(); }; -void CommandZAdd::parseOptions(const std::vector &args, unsigned& index) { - std::unordered_map options = { - {"xx", kZSetXX}, - {"nx", kZSetNX}, - {"ch", kZSetCH}, - {"lt", kZSetLT}, - {"gt", kZSetGT}, - {"incr", kZSetIncr}, - }; +void CommandZAdd::parseOptions(const std::vector &args, unsigned &index) { + std::unordered_map 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]); diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 8adbaf2eb8e..e18333a1cdb 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -96,7 +96,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector= old_score ) { + if (lt && (*mscores)[i].score >= old_score) { continue; } else if (gt && (*mscores)[i].score <= old_score) { continue; From 0d27361ec4065260efd012fb656138ec713ccaca Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 19:04:32 +0800 Subject: [PATCH 09/27] Fix problems of old code about clang-format --- src/commands/redis_cmd.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 71b0dd287f3..4fab728b8bf 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2365,7 +2365,7 @@ class CommandSInterStore : public Commander { class CommandZAdd : public Commander { public: - Status Parse(const std::vector &args) override { + Status Parse(const std::vector &args) override { unsigned index = 2; parseOptions(args, index); if (auto s = validateFlags(); !s.IsOK()) { From 0574b88d432b90de300465cfaf2db9e9dff028df Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 21:08:16 +0800 Subject: [PATCH 10/27] Enable c++ 17 for cpp check --- x.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x.py b/x.py index 5d85d77dede..6a908ff7711 100755 --- a/x.py +++ b/x.py @@ -159,7 +159,7 @@ def cppcheck() -> None: options = ["-x", "c++"] options.append("-U__GNUC__") options.append("--force") - options.append("--std=c++11") + options.append("--std=c++17") # we should run cmake configuration to fetch deps if we want to enable missingInclude options.append("--enable=warning,portability,information") options.append("--error-exitcode=1") From 283634c8538e4979370cbbd0198802a2042f833a Mon Sep 17 00:00:00 2001 From: Twice Date: Fri, 21 Oct 2022 19:34:51 +0800 Subject: [PATCH 11/27] Add mailing list to README (#1023) * Add mailing list to README * Update .asf.yaml * Update README.md Co-authored-by: tison Co-authored-by: tison --- .asf.yaml | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index 75fa0f91e6e..340a4d664f7 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -47,4 +47,4 @@ notifications: issues: issues@kvrocks.apache.org pullrequests: issues@kvrocks.apache.org jobs: builds@kvrocks.apache.org - discussions: dev@kvrocks.apache.org + discussions: issues@kvrocks.apache.org diff --git a/README.md b/README.md index eecd038dda6..37b26e1aac6 100644 --- a/README.md +++ b/README.md @@ -25,8 +25,8 @@ --- -* [Google Group](https://groups.google.com/g/kvrocks) * [Slack Channel](https://join.slack.com/t/kvrockscommunity/shared_invite/zt-p5928e3r-OUAK8SUgC8GOceGM6dAz6w) +* [Mailing List](https://lists.apache.org/list.html?dev@kvrocks.apache.org) ([how to subscribe](https://www.apache.org/foundation/mailinglists.html#subscribing)) **Apache Kvrocks(Incubating)** is a distributed key value NoSQL database that uses RocksDB as storage engine and is compatible with Redis protocol. Kvrocks intends to decrease the cost of memory and increase the capacity while compared to Redis. The design of replication and storage was inspired by `rocksplicator` and `blackwidow`. From d985bba62d789cbf474a0587f9a10eca0f067772 Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 21 Oct 2022 21:33:52 +0800 Subject: [PATCH 12/27] change the code for bad cppcheck --- src/commands/redis_cmd.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 4fab728b8bf..f769f3ef975 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2429,7 +2429,8 @@ void CommandZAdd::parseOptions(const std::vector &args, unsigned &i constexpr unsigned max_index = 6; for (unsigned i = 2; i < max_index; i++) { auto option = Util::ToLower(args[i]); - if (auto it = options.find(option); it != options.end()) { + auto it = options.find(option); + if (it != options.end()) { flags_ |= it->second; index++; } else { From af27eac72ba0ed7b4331840d107b2c3549013331 Mon Sep 17 00:00:00 2001 From: manchurio Date: Sat, 22 Oct 2022 17:16:27 +0800 Subject: [PATCH 13/27] format go test case --- tests/gocase/unit/type/zset/zset_test.go | 51 ++++++++++++------------ 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index 4c0ae58773f..d2b173b9327 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -99,42 +99,43 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s require.Contains(t, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}, {Member: "adc"}}}).Err(), "INCR option supports a single increment-element pair") }) + t.Run(fmt.Sprintf("ZSET ZADD IncrMixedOtherOptions - %s", encoding), func(t *testing.T) { rdb.Del(ctx, "ztmp") - require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) rdb.Del(ctx, "ztmp") - require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{XX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) - require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{XX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) rdb.Del(ctx, "ztmp") - require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, 3.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) - require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Err()) + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 3.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Err()) rdb.Del(ctx, "ztmp") - require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX:true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) - require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true,Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) + require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val()) + require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) }) t.Run(fmt.Sprintf("ZSET ZADD LT/GT with other options - %s", encoding), func(t *testing.T) { rdb.Del(ctx, "ztmp") require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 100}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 100}}}).Val()) require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "lt", "gt", "1", "m1", "2", "m2").Err(), "GT, LT, and/or NX options at the same time are not compatible") rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.2}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT:true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 0.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.2}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 0.5}}}).Val()) rdb.Del(ctx, "newAbc1", "newAbc2") require.Equal(t, int64(2), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Ch: true, Members: []redis.Z{{Member: "abc", Score: 0.5}, {Member: "newAbc1", Score: 10}, {Member: "newAbc2"}}}).Val()) @@ -142,10 +143,10 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s t.Run(fmt.Sprintf("ZSET ZADD NX/XX option supports a single pair - %s", encoding), func(t *testing.T) { rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "nx", "xx", "1", "m1", "2", "m2").Err(), "XX and NX options at the same time are not compatible") @@ -155,8 +156,8 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s "GT, LT, and/or NX options at the same time are not compatible") rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX:true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) }) t.Run(fmt.Sprintf("ZSET element can't be set to NaN with ZADD - %s", encoding), func(t *testing.T) { From 5f589dce8598b068060ea33880491320b2cbcef7 Mon Sep 17 00:00:00 2001 From: manchurio Date: Sat, 22 Oct 2022 17:29:51 +0800 Subject: [PATCH 14/27] Add duplicate nx/xx to zadd --- src/commands/redis_cmd.cc | 3 +-- tests/gocase/unit/type/zset/zset_test.go | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index f769f3ef975..ab1923b9d81 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2426,8 +2426,7 @@ class CommandZAdd : public Commander { void CommandZAdd::parseOptions(const std::vector &args, unsigned &index) { std::unordered_map 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++) { + for (unsigned i = 2; i < args.size(); i++) { auto option = Util::ToLower(args[i]); auto it = options.find(option); if (it != options.end()) { diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index d2b173b9327..68c5ef0fd59 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -101,6 +101,11 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s }) t.Run(fmt.Sprintf("ZSET ZADD IncrMixedOtherOptions - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, "1.5", rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Val()) + require.Equal(t, redis.Nil, rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Err()) + require.Equal(t, "3", rdb.Do(ctx, "zadd", "ztmp", "xx", "xx", "xx", "xx", "incr", "1.5", "abc").Val()) + rdb.Del(ctx, "ztmp") require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err()) From 79f86078fd6125d46fd2c30849c257291e3cc974 Mon Sep 17 00:00:00 2001 From: manchurio Date: Sun, 23 Oct 2022 10:58:03 +0800 Subject: [PATCH 15/27] Fix problems of code style. --- src/commands/redis_cmd.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index d8fb984618e..65c0ef4d36b 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2389,8 +2389,9 @@ class CommandZAdd : public Commander { if (auto left = (args.size() - index); left > 0) { if ((flags_ & kZSetIncr) && left != 2) { return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair"); - } else if (left % 2 != 0) + } else if (left % 2 != 0) { return Status(Status::RedisParseErr, errInvalidSyntax); + } } try { for (unsigned i = index; i < args.size(); i += 2) { @@ -2409,12 +2410,12 @@ 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], flags_, &member_scores_, &ret); if (!s.ok()) { return Status(Status::RedisExecErr, s.ToString()); } + bool incr = (flags_ & kZSetIncr) != 0; if (incr) { auto new_score = member_scores_[0].score; bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT); @@ -2435,7 +2436,7 @@ class CommandZAdd : public Commander { uint8_t flags_ = 0; void parseOptions(const std::vector &args, unsigned &index); - Status validateFlags(); + Status validateFlags() const; }; void CommandZAdd::parseOptions(const std::vector &args, unsigned &index) { @@ -2453,9 +2454,9 @@ void CommandZAdd::parseOptions(const std::vector &args, unsigned &i } } -Status CommandZAdd::validateFlags() { +Status CommandZAdd::validateFlags() const { if (!flags_) { - return {}; + return Status::OK(); } bool nx = (flags_ & kZSetNX) != 0; bool xx = (flags_ & kZSetXX) != 0; @@ -2473,7 +2474,7 @@ Status CommandZAdd::validateFlags() { if (gt && nx) { return Status(Status::RedisParseErr, errZSetLTGTNX); } - return {}; + return Status::OK(); } class CommandZCount : public Commander { From 6ac21ac7c14067eba10cdf3f32511f9c59223b98 Mon Sep 17 00:00:00 2001 From: manchurio Date: Sun, 23 Oct 2022 11:09:39 +0800 Subject: [PATCH 16/27] Fix NX problem. --- src/types/redis_zset.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index c8d178ce3b4..238de3a5844 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -76,13 +76,13 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector 0) { - if (flags & kZSetNX) { - continue; - } std::string old_score_bytes; s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes); if (!s.ok() && !s.IsNotFound()) return s; if (s.ok()) { + if (!s.IsNotFound() && (flags & kZSetNX)) { + continue; + } double old_score = DecodeDouble(old_score_bytes.data()); if (flags & kZSetIncr) { if (lt && (*mscores)[i].score >= 0) { From 93186a1368546f8ccde04e3c1e2f797dbdf3fb4b Mon Sep 17 00:00:00 2001 From: manchurio Date: Sun, 23 Oct 2022 13:58:16 +0800 Subject: [PATCH 17/27] Add NX with multiple pairs in zadd unit tests --- tests/gocase/unit/type/zset/zset_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index 68c5ef0fd59..e7caee09c42 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -147,6 +147,10 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s }) t.Run(fmt.Sprintf("ZSET ZADD NX/XX option supports a single pair - %s", encoding), func(t *testing.T) { + rdb.Del(ctx, "ztmp") + require.Equal(t, int64(2), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "a", Score: 1}, {Member: "b", Score: 2}}}).Val()) + require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "c", Score: 3}}}).Val()) + rdb.Del(ctx, "ztmp") require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) From 199eb21182d18631d2cb1d8ba50f803417b8cac8 Mon Sep 17 00:00:00 2001 From: manchurio Date: Sun, 23 Oct 2022 14:41:56 +0800 Subject: [PATCH 18/27] change permission of x.py to 755 --- x.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 x.py diff --git a/x.py b/x.py old mode 100644 new mode 100755 From ba43865444c2fea0aa6fa74b2ea911b8819afed5 Mon Sep 17 00:00:00 2001 From: manchurio Date: Mon, 24 Oct 2022 12:27:59 +0800 Subject: [PATCH 19/27] Change parseOptions to parseFlags --- src/commands/redis_cmd.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 65c0ef4d36b..ccf61a93229 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2382,7 +2382,7 @@ class CommandZAdd : public Commander { public: Status Parse(const std::vector &args) override { unsigned index = 2; - parseOptions(args, index); + parseFlags(args, index); if (auto s = validateFlags(); !s.IsOK()) { return s; } @@ -2435,11 +2435,11 @@ class CommandZAdd : public Commander { std::vector member_scores_; uint8_t flags_ = 0; - void parseOptions(const std::vector &args, unsigned &index); + void parseFlags(const std::vector &args, unsigned &index); Status validateFlags() const; }; -void CommandZAdd::parseOptions(const std::vector &args, unsigned &index) { +void CommandZAdd::parseFlags(const std::vector &args, unsigned &index) { std::unordered_map options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH}, {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}}; for (unsigned i = 2; i < args.size(); i++) { From 9daea188ce6735e5b1f98fd7b43e82301dbc383b Mon Sep 17 00:00:00 2001 From: manchurio Date: Mon, 24 Oct 2022 15:14:51 +0800 Subject: [PATCH 20/27] Refactor flags from uint8_t to struct ZAddFlags --- src/commands/redis_cmd.cc | 17 +++++++---------- src/types/redis_geo.cc | 4 ++-- src/types/redis_zset.cc | 16 ++++++++-------- src/types/redis_zset.h | 23 ++++++++++++++++++++++- tests/cppunit/compact_test.cc | 2 +- tests/cppunit/disk_test.cc | 2 +- tests/cppunit/t_zset_test.cc | 30 +++++++++++++++--------------- 7 files changed, 56 insertions(+), 38 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index ccf61a93229..c8117e5de66 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2387,7 +2387,7 @@ class CommandZAdd : public Commander { return s; } if (auto left = (args.size() - index); left > 0) { - if ((flags_ & kZSetIncr) && left != 2) { + if (flags_.HasIncr() && left != 2) { return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair"); } else if (left % 2 != 0) { return Status(Status::RedisParseErr, errInvalidSyntax); @@ -2415,10 +2415,10 @@ class CommandZAdd : public Commander { if (!s.ok()) { return Status(Status::RedisExecErr, s.ToString()); } - bool incr = (flags_ & kZSetIncr) != 0; + bool incr = flags_.HasIncr(); if (incr) { auto new_score = member_scores_[0].score; - bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT); + bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT(); if ((nx || xx || lt || gt) && old_score == new_score && ret == 0) { // not the first time using incr && score not changed *output = Redis::NilString(); @@ -2433,7 +2433,7 @@ class CommandZAdd : public Commander { private: std::vector member_scores_; - uint8_t flags_ = 0; + ZAddFlags flags_{0}; void parseFlags(const std::vector &args, unsigned &index); Status validateFlags() const; @@ -2446,7 +2446,7 @@ void CommandZAdd::parseFlags(const std::vector &args, unsigned &ind auto option = Util::ToLower(args[i]); auto it = options.find(option); if (it != options.end()) { - flags_ |= it->second; + flags_.SetFlag(it->second); index++; } else { break; @@ -2455,13 +2455,10 @@ void CommandZAdd::parseFlags(const std::vector &args, unsigned &ind } Status CommandZAdd::validateFlags() const { - if (!flags_) { + if (!flags_.HasFlag()) { return Status::OK(); } - bool nx = (flags_ & kZSetNX) != 0; - bool xx = (flags_ & kZSetXX) != 0; - bool lt = (flags_ & kZSetLT) != 0; - bool gt = (flags_ & kZSetGT) != 0; + bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT(); if (nx && xx) { return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible"); } diff --git a/src/types/redis_geo.cc b/src/types/redis_geo.cc index 293ca3776ba..0400eddceaf 100644 --- a/src/types/redis_geo.cc +++ b/src/types/redis_geo.cc @@ -35,7 +35,7 @@ rocksdb::Status Geo::Add(const Slice &user_key, std::vector *geo_point GeoHashFix52Bits bits = GeoHashHelper::Align52Bits(hash); member_scores.emplace_back(MemberScore{geo_point.member, static_cast(bits)}); } - return ZSet::Add(user_key, 0, &member_scores, ret); + return ZSet::Add(user_key, ZAddFlags::Default(), &member_scores, ret); } rocksdb::Status Geo::Dist(const Slice &user_key, const Slice &member_1, const Slice &member_2, double *dist) { @@ -120,7 +120,7 @@ rocksdb::Status Geo::Radius(const Slice &user_key, double longitude, double lati member_scores.emplace_back(MemberScore{geo_point.member, score}); } int ret; - ZSet::Add(store_key, 0, &member_scores, &ret); + ZSet::Add(store_key, ZAddFlags::Default(), &member_scores, &ret); } } diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 238de3a5844..271cc86ce4e 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -37,7 +37,7 @@ rocksdb::Status ZSet::GetMetadata(const Slice &ns_key, ZSetMetadata *metadata) { return Database::GetMetadata(kRedisZSet, ns_key, metadata); } -rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector *mscores, int *ret) { +rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector *mscores, int *ret) { *ret = 0; std::string ns_key; @@ -72,19 +72,19 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector 0) { std::string old_score_bytes; s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes); if (!s.ok() && !s.IsNotFound()) return s; if (s.ok()) { - if (!s.IsNotFound() && (flags & kZSetNX)) { + if (!s.IsNotFound() && flags.HasNX()) { continue; } double old_score = DecodeDouble(old_score_bytes.data()); - if (flags & kZSetIncr) { + if (flags.HasIncr()) { if (lt && (*mscores)[i].score >= 0) { continue; } else if (gt && (*mscores)[i].score <= 0) { @@ -116,7 +116,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vectorWrite(storage_->DefaultWriteOptions(), &batch); @@ -162,7 +162,7 @@ rocksdb::Status ZSet::IncrBy(const Slice &user_key, const Slice &member, double int ret; std::vector mscores; mscores.emplace_back(MemberScore{member.ToString(), increment}); - rocksdb::Status s = Add(user_key, kZSetIncr, &mscores, &ret); + rocksdb::Status s = Add(user_key, ZAddFlags::Incr(), &mscores, &ret); if (!s.ok()) return s; *score = mscores[0].score; return rocksdb::Status::OK(); diff --git a/src/types/redis_zset.h b/src/types/redis_zset.h index f1952e43434..a79feed7b7a 100644 --- a/src/types/redis_zset.h +++ b/src/types/redis_zset.h @@ -87,13 +87,34 @@ enum ZSetFlags { kZSetCH = 1 << 7, }; +typedef struct ZAddFlags { + explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {} + + bool HasNX() const { return (flags & kZSetNX) != 0; } + bool HasXX() const { return (flags & kZSetXX) != 0; } + bool HasLT() const { return (flags & kZSetLT) != 0; } + bool HasGT() const { return (flags & kZSetGT) != 0; } + bool HasCH() const { return (flags & kZSetCH) != 0; } + bool HasIncr() const { return (flags & kZSetIncr) != 0; } + bool HasFlag() const { return flags != 0; } + + void SetFlag(ZSetFlags setFlags) { flags |= setFlags;} + + static const ZAddFlags Incr() { return ZAddFlags{kZSetIncr};} + + static const ZAddFlags Default() { return ZAddFlags{0};} + +private: + uint8_t flags = 0; +} ZAddFlags; + namespace Redis { class ZSet : public SubKeyScanner { public: explicit ZSet(Engine::Storage *storage, const std::string &ns) : SubKeyScanner(storage, ns), score_cf_handle_(storage->GetCFHandle("zset_score")) {} - rocksdb::Status Add(const Slice &user_key, uint8_t flags, std::vector *mscores, int *ret); + rocksdb::Status Add(const Slice &user_key, ZAddFlags flags, std::vector *mscores, int *ret); rocksdb::Status Card(const Slice &user_key, int *ret); rocksdb::Status Count(const Slice &user_key, const ZRangeSpec &spec, int *ret); rocksdb::Status IncrBy(const Slice &user_key, const Slice &member, double increment, double *score); diff --git a/tests/cppunit/compact_test.cc b/tests/cppunit/compact_test.cc index 4042e885ed9..1c75e04f822 100644 --- a/tests/cppunit/compact_test.cc +++ b/tests/cppunit/compact_test.cc @@ -75,7 +75,7 @@ TEST(Compact, Filter) { auto zset = std::make_unique(storage_.get(), ns); std::string expired_zset_key = "expire_zset_key"; std::vector member_scores = {MemberScore{"z1", 1.1}, MemberScore{"z2", 0.4}}; - zset->Add(expired_zset_key, 0, &member_scores, &ret); + zset->Add(expired_zset_key, ZAddFlags::Default(), &member_scores, &ret); zset->Expire(expired_zset_key, 1); // expired usleep(10000); diff --git a/tests/cppunit/disk_test.cc b/tests/cppunit/disk_test.cc index ca252df43fa..5472c5b9d5e 100644 --- a/tests/cppunit/disk_test.cc +++ b/tests/cppunit/disk_test.cc @@ -148,7 +148,7 @@ TEST_F(RedisDiskTest, ZsetDisk) { mscores[i].score = 1.0 * value_size[int(values_.size()) - i - 1]; approximate_size += (key_.size() + 8 + mscores[i].member.size() + 8) * 2; } - rocksdb::Status s = zset->Add(key_, 0, &mscores, &ret); + rocksdb::Status s = zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_TRUE(s.ok() && ret == 5); uint64_t key_size = 0; EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok()); diff --git a/tests/cppunit/t_zset_test.cc b/tests/cppunit/t_zset_test.cc index d750ab0a29c..288fbe1142c 100644 --- a/tests/cppunit/t_zset_test.cc +++ b/tests/cppunit/t_zset_test.cc @@ -47,14 +47,14 @@ TEST_F(RedisZSetTest, Add) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(static_cast(fields_.size()), ret); for (size_t i = 0; i < fields_.size(); i++) { double got; rocksdb::Status s = zset->Score(key_, fields_[i], &got); EXPECT_EQ(scores_[i], got); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(ret, 0); zset->Del(key_); } @@ -65,7 +65,7 @@ TEST_F(RedisZSetTest, IncrBy) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); for (size_t i = 0; i < fields_.size(); i++) { double increment = 12.3, score; @@ -81,7 +81,7 @@ TEST_F(RedisZSetTest, Remove) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); zset->Remove(key_, fields_, &ret); EXPECT_EQ(fields_.size(), ret); @@ -100,7 +100,7 @@ TEST_F(RedisZSetTest, Range) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } int count = mscores.size() - 1; - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); zset->Range(key_, 0, -2, 0, &mscores); EXPECT_EQ(mscores.size(), count); @@ -118,7 +118,7 @@ TEST_F(RedisZSetTest, RevRange) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } int count = mscores.size() - 1; - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(static_cast(fields_.size()), ret); zset->Range(key_, 0, -2, kZSetReversed, &mscores); EXPECT_EQ(mscores.size(), count); @@ -135,7 +135,7 @@ TEST_F(RedisZSetTest, PopMin) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(static_cast(fields_.size()), ret); zset->Pop(key_, mscores.size() - 1, true, &mscores); for (size_t i = 0; i < mscores.size(); i++) { @@ -154,7 +154,7 @@ TEST_F(RedisZSetTest, PopMax) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(static_cast(fields_.size()), ret); zset->Pop(key_, mscores.size() - 1, false, &mscores); for (size_t i = 0; i < mscores.size(); i++) { @@ -171,7 +171,7 @@ TEST_F(RedisZSetTest, RangeByLex) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); ZRangeLexSpec spec; @@ -227,7 +227,7 @@ TEST_F(RedisZSetTest, RangeByScore) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); // test case: inclusive the min and max score @@ -275,7 +275,7 @@ TEST_F(RedisZSetTest, RangeByScoreWithLimit) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); ZRangeSpec spec; @@ -296,7 +296,7 @@ TEST_F(RedisZSetTest, RemRangeByScore) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); ZRangeSpec spec; spec.min = scores_[0]; @@ -315,7 +315,7 @@ TEST_F(RedisZSetTest, RemoveRangeByRank) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); zset->RemoveRangeByRank(key_, 0, fields_.size() - 2, &ret); EXPECT_EQ(fields_.size() - 1, ret); @@ -329,7 +329,7 @@ TEST_F(RedisZSetTest, RemoveRevRangeByRank) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(fields_.size(), ret); zset->RemoveRangeByRank(key_, 0, fields_.size() - 2, &ret); EXPECT_EQ(static_cast(fields_.size() - 1), ret); @@ -343,7 +343,7 @@ TEST_F(RedisZSetTest, Rank) { for (size_t i = 0; i < fields_.size(); i++) { mscores.emplace_back(MemberScore{fields_[i].ToString(), scores_[i]}); } - zset->Add(key_, 0, &mscores, &ret); + zset->Add(key_, ZAddFlags::Default(), &mscores, &ret); EXPECT_EQ(static_cast(fields_.size()), ret); for (size_t i = 0; i < fields_.size(); i++) { From a429eaa63d1c13d9e3313bdfaddb2616a99ea9cc Mon Sep 17 00:00:00 2001 From: manchurio Date: Mon, 24 Oct 2022 15:18:36 +0800 Subject: [PATCH 21/27] Merge multiple conditions into one line. --- src/commands/redis_cmd.cc | 8 +------- src/types/redis_zset.cc | 8 ++------ 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index c8117e5de66..b83154b86ea 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2462,13 +2462,7 @@ Status CommandZAdd::validateFlags() const { if (nx && xx) { return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible"); } - if (lt && gt) { - return Status(Status::RedisParseErr, errZSetLTGTNX); - } - if (lt && nx) { - return Status(Status::RedisParseErr, errZSetLTGTNX); - } - if (gt && nx) { + if ((lt && gt) || (lt && nx) || (gt && nx)) { return Status(Status::RedisParseErr, errZSetLTGTNX); } return Status::OK(); diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 271cc86ce4e..201213a3380 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -85,9 +85,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector= 0) { - continue; - } else if (gt && (*mscores)[i].score <= 0) { + if ((lt && (*mscores)[i].score >= 0) || (gt && (*mscores)[i].score <= 0)) { continue; } (*mscores)[i].score += old_score; @@ -96,9 +94,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector= old_score) { - continue; - } else if (gt && (*mscores)[i].score <= old_score) { + if ((lt && (*mscores)[i].score >= old_score) || (gt && (*mscores)[i].score <= old_score)) { continue; } old_score_bytes.append((*mscores)[i].member); From 4688daa4fe1ce93f19848caabc06dc5c44867402 Mon Sep 17 00:00:00 2001 From: manchurio Date: Mon, 24 Oct 2022 15:24:39 +0800 Subject: [PATCH 22/27] Using EqualValues instead of Equals to avoid type cast. --- tests/gocase/unit/type/zset/zset_test.go | 34 ++++++++++++------------ 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index e7caee09c42..7dae9e625b2 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -129,33 +129,33 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s t.Run(fmt.Sprintf("ZSET ZADD LT/GT with other options - %s", encoding), func(t *testing.T) { rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 100}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{GT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 100}}}).Val()) require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "lt", "gt", "1", "m1", "2", "m2").Err(), "GT, LT, and/or NX options at the same time are not compatible") rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.2}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 0.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.2}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{LT: true, Ch: false, Members: []redis.Z{{Member: "abc", Score: 0.5}}}).Val()) rdb.Del(ctx, "newAbc1", "newAbc2") - require.Equal(t, int64(2), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Ch: true, Members: []redis.Z{{Member: "abc", Score: 0.5}, {Member: "newAbc1", Score: 10}, {Member: "newAbc2"}}}).Val()) + require.EqualValues(t, 2, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Ch: true, Members: []redis.Z{{Member: "abc", Score: 0.5}, {Member: "newAbc1", Score: 10}, {Member: "newAbc2"}}}).Val()) }) t.Run(fmt.Sprintf("ZSET ZADD NX/XX option supports a single pair - %s", encoding), func(t *testing.T) { rdb.Del(ctx, "ztmp") - require.Equal(t, int64(2), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "a", Score: 1}, {Member: "b", Score: 2}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "c", Score: 3}}}).Val()) + require.EqualValues(t, 2, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "a", Score: 1}, {Member: "b", Score: 2}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "c", Score: 3}}}).Val()) rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{XX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 2.5}}}).Val()) require.Contains(t, rdb.Do(ctx, "zadd", "ztmp", "nx", "xx", "1", "m1", "2", "m2").Err(), "XX and NX options at the same time are not compatible") @@ -165,8 +165,8 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s "GT, LT, and/or NX options at the same time are not compatible") rdb.Del(ctx, "ztmp") - require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) - require.Equal(t, int64(0), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Ch: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) + require.EqualValues(t, 0, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val()) }) t.Run(fmt.Sprintf("ZSET element can't be set to NaN with ZADD - %s", encoding), func(t *testing.T) { From b58ae86f11bd6f7c90560ef7df93ef8382be39db Mon Sep 17 00:00:00 2001 From: manchurio Date: Mon, 24 Oct 2022 15:38:18 +0800 Subject: [PATCH 23/27] Fix clang-format. --- src/types/redis_zset.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/types/redis_zset.h b/src/types/redis_zset.h index a79feed7b7a..ebad2a03234 100644 --- a/src/types/redis_zset.h +++ b/src/types/redis_zset.h @@ -98,13 +98,13 @@ typedef struct ZAddFlags { bool HasIncr() const { return (flags & kZSetIncr) != 0; } bool HasFlag() const { return flags != 0; } - void SetFlag(ZSetFlags setFlags) { flags |= setFlags;} + void SetFlag(ZSetFlags setFlags) { flags |= setFlags; } - static const ZAddFlags Incr() { return ZAddFlags{kZSetIncr};} + static const ZAddFlags Incr() { return ZAddFlags{kZSetIncr}; } - static const ZAddFlags Default() { return ZAddFlags{0};} + static const ZAddFlags Default() { return ZAddFlags{0}; } -private: + private: uint8_t flags = 0; } ZAddFlags; From 7c7b6045d99158679758389469249f66ab969aba Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 27 Oct 2022 16:58:46 +0800 Subject: [PATCH 24/27] No need for additional local variables --- src/commands/redis_cmd.cc | 13 +++++-------- src/types/redis_zset.cc | 7 ++----- src/types/redis_zset.h | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index dc0794306fa..3d303cea101 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2412,11 +2412,9 @@ class CommandZAdd : public Commander { if (!s.ok()) { return Status(Status::RedisExecErr, s.ToString()); } - bool incr = flags_.HasIncr(); - if (incr) { + if (flags_.HasIncr()) { auto new_score = member_scores_[0].score; - bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT(); - if ((nx || xx || lt || gt) && old_score == new_score && + if ((flags_.HasNX() || flags_.HasXX() || flags_.HasLT() || flags_.HasGT()) && old_score == new_score && ret == 0) { // not the first time using incr && score not changed *output = Redis::NilString(); return Status::OK(); @@ -2452,14 +2450,13 @@ void CommandZAdd::parseFlags(const std::vector &args, unsigned &ind } Status CommandZAdd::validateFlags() const { - if (!flags_.HasFlag()) { + if (!flags_.HasAnyFlags()) { return Status::OK(); } - bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT(); - if (nx && xx) { + if (flags_.HasNX() && flags_.HasXX()) { return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible"); } - if ((lt && gt) || (lt && nx) || (gt && nx)) { + if ((flags_.HasLT() && flags_.HasGT()) || (flags_.HasLT() && flags_.HasNX()) || (flags_.HasGT() && flags_.HasNX())) { return Status(Status::RedisParseErr, errZSetLTGTNX); } return Status::OK(); diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 201213a3380..3bc09460cd4 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -72,9 +72,6 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector 0) { std::string old_score_bytes; s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes); @@ -85,7 +82,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector= 0) || (gt && (*mscores)[i].score <= 0)) { + if ((flags.HasLT() && (*mscores)[i].score >= 0) || (flags.HasGT() && (*mscores)[i].score <= 0)) { continue; } (*mscores)[i].score += old_score; @@ -94,7 +91,7 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector= old_score) || (gt && (*mscores)[i].score <= old_score)) { + if ((flags.HasLT() && (*mscores)[i].score >= old_score) || (flags.HasGT() && (*mscores)[i].score <= old_score)) { continue; } old_score_bytes.append((*mscores)[i].member); diff --git a/src/types/redis_zset.h b/src/types/redis_zset.h index ebad2a03234..c8e12aa880e 100644 --- a/src/types/redis_zset.h +++ b/src/types/redis_zset.h @@ -96,7 +96,7 @@ typedef struct ZAddFlags { bool HasGT() const { return (flags & kZSetGT) != 0; } bool HasCH() const { return (flags & kZSetCH) != 0; } bool HasIncr() const { return (flags & kZSetIncr) != 0; } - bool HasFlag() const { return flags != 0; } + bool HasAnyFlags() const { return flags != 0; } void SetFlag(ZSetFlags setFlags) { flags |= setFlags; } From f704d6f0c935b52c86913f478fe2b62ee786682d Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 27 Oct 2022 17:58:30 +0800 Subject: [PATCH 25/27] clang format --- src/types/redis_zset.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 3bc09460cd4..558f25ab27b 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -91,7 +91,8 @@ rocksdb::Status ZSet::Add(const Slice &user_key, ZAddFlags flags, std::vector= old_score) || (flags.HasGT() && (*mscores)[i].score <= old_score)) { + if ((flags.HasLT() && (*mscores)[i].score >= old_score) || + (flags.HasGT() && (*mscores)[i].score <= old_score)) { continue; } old_score_bytes.append((*mscores)[i].member); From 8c178b71954b34661c59a51470de99c83c0d06ee Mon Sep 17 00:00:00 2001 From: manchurio Date: Thu, 27 Oct 2022 18:34:04 +0800 Subject: [PATCH 26/27] use class instead of typedef struct --- src/types/redis_zset.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/types/redis_zset.h b/src/types/redis_zset.h index c8e12aa880e..b97e98be24f 100644 --- a/src/types/redis_zset.h +++ b/src/types/redis_zset.h @@ -87,7 +87,8 @@ enum ZSetFlags { kZSetCH = 1 << 7, }; -typedef struct ZAddFlags { +class ZAddFlags { + public: explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {} bool HasNX() const { return (flags & kZSetNX) != 0; } @@ -106,7 +107,7 @@ typedef struct ZAddFlags { private: uint8_t flags = 0; -} ZAddFlags; +}; namespace Redis { From 16829bffe04b5d53420ee094f3729c1907d65539 Mon Sep 17 00:00:00 2001 From: manchurio Date: Fri, 28 Oct 2022 01:11:15 +0800 Subject: [PATCH 27/27] Fix no pair bugs in zadd --- src/commands/redis_cmd.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/redis_cmd.cc b/src/commands/redis_cmd.cc index 3d303cea101..d2e03e88067 100644 --- a/src/commands/redis_cmd.cc +++ b/src/commands/redis_cmd.cc @@ -2383,10 +2383,10 @@ class CommandZAdd : public Commander { if (auto s = validateFlags(); !s.IsOK()) { return s; } - if (auto left = (args.size() - index); left > 0) { + if (auto left = (args.size() - index); left >= 0) { if (flags_.HasIncr() && left != 2) { return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair"); - } else if (left % 2 != 0) { + } else if (left % 2 != 0 || left == 0) { return Status(Status::RedisParseErr, errInvalidSyntax); } }