From 6e8ab73d9ef45a53353132d0918a7d63bfc27a47 Mon Sep 17 00:00:00 2001 From: Redhal Date: Tue, 1 Nov 2022 23:43:42 +0100 Subject: [PATCH 1/5] refactors ZRange fns + adds ZRevRangebylex + tests --- src/server/zset_family.cc | 228 +++++++++++++++++---------------- src/server/zset_family.h | 14 +- src/server/zset_family_test.cc | 63 +++++++++ 3 files changed, 191 insertions(+), 114 deletions(-) diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index b55d234deeb4..01cbadcf806e 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -16,9 +16,9 @@ extern "C" { #include "facade/error.h" #include "server/command_registry.h" #include "server/conn_context.h" +#include "server/container_utils.h" #include "server/engine_shard_set.h" #include "server/transaction.h" -#include "server/container_utils.h" namespace dfly { @@ -122,11 +122,7 @@ OpResult FindZEntry(const ZParams& zparams, const OpArgs& op_args return it; } -enum class Action { - RANGE = 0, - REMOVE = 1, - POP = 2 -}; +enum class Action { RANGE = 0, REMOVE = 1, POP = 2 }; class IntervalVisitor { public: @@ -269,10 +265,13 @@ void IntervalVisitor::operator()(ZSetFamily::TopNScored sc) { } void IntervalVisitor::ActionRange(unsigned start, unsigned end) { - container_utils::IterateSortedSet(zobj_, [this](container_utils::ContainerEntry ce, double score){ - result_.emplace_back(ce.ToString(), score); - return true; - }, start, end, params_.reverse, params_.with_scores); + container_utils::IterateSortedSet( + zobj_, + [this](container_utils::ContainerEntry ce, double score) { + result_.emplace_back(ce.ToString(), score); + return true; + }, + start, end, params_.reverse, params_.with_scores); } void IntervalVisitor::ActionRange(const zrangespec& range) { @@ -513,9 +512,9 @@ void IntervalVisitor::PopListPack(ZSetFamily::TopNScored sc) { long long vlong = 0; if (params_.reverse) { - eptr = lpSeek(zl,-2); + eptr = lpSeek(zl, -2); } else { - eptr = lpSeek(zl,0); + eptr = lpSeek(zl, 0); } /* Get score pointer for the first element. */ @@ -537,11 +536,11 @@ void IntervalVisitor::PopListPack(ZSetFamily::TopNScored sc) { if (params_.reverse) { /* If the number of elements to delete is greater than the listpack length, * we set the start to 0 because lpseek fails to search beyond length in reverse */ - start = (2*sc > lpLength(zl)) ? 0 : -2*sc; + start = (2 * sc > lpLength(zl)) ? 0 : -2 * sc; } /* We can finally delete the elements */ - zobj_->ptr = lpDeleteRange(zl, start, 2*sc); + zobj_->ptr = lpDeleteRange(zl, start, 2 * sc); } void IntervalVisitor::PopSkipList(ZSetFamily::TopNScored sc) { @@ -1204,7 +1203,44 @@ void ZSetFamily::ZLexCount(CmdArgList args, ConnectionContext* cntx) { } void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { - ZRangeGeneric(std::move(args), false, cntx); + RangeParams::INTERVALTYPE interval_type = RangeParams::INTERVALTYPE::RANK; + RangeParams range_params; + + for (size_t i = 4; i < args.size(); ++i) { + ToUpper(&args[i]); + + string_view cur_arg = ArgS(args, i); + if (cur_arg == "BYSCORE") { + if (interval_type == RangeParams::INTERVALTYPE::LEX) { + return (*cntx)->SendError("BYSCORE and BYLEX options are not compatible"); + } + interval_type = RangeParams::INTERVALTYPE::SCORE; + } else if (cur_arg == "BYLEX") { + if (interval_type == RangeParams::INTERVALTYPE::SCORE) { + return (*cntx)->SendError("BYSCORE and BYLEX options are not compatible"); + } + interval_type = RangeParams::INTERVALTYPE::LEX; + } else if (cur_arg == "REV") { + range_params.reverse = true; + } else if (cur_arg == "WITHSCORES") { + range_params.with_scores = true; + } else if (cur_arg == "LIMIT") { + if (i + 3 != args.size()) { + return (*cntx)->SendError(kSyntaxErr); + } + string_view os = ArgS(args, i + 1); + string_view cs = ArgS(args, i + 2); + if (!SimpleAtoi(os, &range_params.offset) || !SimpleAtoi(cs, &range_params.limit)) { + return (*cntx)->SendError(kInvalidIntErr); + } + i += 3; + } else { + return cntx->reply_builder()->SendError(absl::StrCat("unsupported option ", cur_arg)); + } + } + + range_params.interval_type = interval_type; + ZRangeGeneric(std::move(args), range_params, cntx); } void ZSetFamily::ZRank(CmdArgList args, ConnectionContext* cntx) { @@ -1212,23 +1248,25 @@ void ZSetFamily::ZRank(CmdArgList args, ConnectionContext* cntx) { } void ZSetFamily::ZRevRange(CmdArgList args, ConnectionContext* cntx) { - ZRangeGeneric(std::move(args), true, cntx); -} - -void ZSetFamily::ZRevRangeByScore(CmdArgList args, ConnectionContext* cntx) { - string_view key = ArgS(args, 1); - string_view min_s = ArgS(args, 2); - string_view max_s = ArgS(args, 3); - RangeParams range_params; range_params.reverse = true; - args.remove_prefix(4); - if (!ParseRangeByScoreParams(args, &range_params)) { - return (*cntx)->SendError(kSyntaxErr); + for (size_t i = 4; i < args.size(); ++i) { + ToUpper(&args[i]); + + string_view cur_arg = ArgS(args, i); + if (cur_arg == "WITHSCORES") { + range_params.with_scores = true; + } else { + return cntx->reply_builder()->SendError(absl::StrCat("unsupported option ", cur_arg)); + } } - ZRangeByScoreInternal(key, min_s, max_s, range_params, cntx); + ZRangeGeneric(std::move(args), range_params, cntx); +} + +void ZSetFamily::ZRevRangeByScore(CmdArgList args, ConnectionContext* cntx) { + ZRangeByScoreInternal(std::move(args), true, cntx); } void ZSetFamily::ZRevRank(CmdArgList args, ConnectionContext* cntx) { @@ -1236,12 +1274,20 @@ void ZSetFamily::ZRevRank(CmdArgList args, ConnectionContext* cntx) { } void ZSetFamily::ZRangeByLex(CmdArgList args, ConnectionContext* cntx) { - string_view key = ArgS(args, 1); - string_view min_s = ArgS(args, 2); - string_view max_s = ArgS(args, 3); + ZRangeByLexInternal(std::move(args), false, cntx); +} +void ZSetFamily::ZRevRangeByLex(CmdArgList args, ConnectionContext* cntx) { + ZRangeByLexInternal(std::move(args), true, cntx); +} + +void ZSetFamily::ZRangeByLexInternal(CmdArgList args, bool reverse, ConnectionContext* cntx) { uint32_t offset = 0; uint32_t count = kuint32max; + RangeParams range_params; + range_params.interval_type = RangeParams::INTERVALTYPE::LEX; + range_params.reverse = reverse; + if (args.size() > 4) { if (args.size() != 7) return (*cntx)->SendError(kSyntaxErr); @@ -1251,42 +1297,18 @@ void ZSetFamily::ZRangeByLex(CmdArgList args, ConnectionContext* cntx) { return (*cntx)->SendError(kSyntaxErr); string_view os = ArgS(args, 5); string_view cs = ArgS(args, 6); - if (!SimpleAtoi(os, &count) || !SimpleAtoi(cs, &count)) { + if (!SimpleAtoi(os, &offset) || !SimpleAtoi(cs, &count)) { return (*cntx)->SendError(kInvalidIntErr); } } + range_params.offset = offset; + range_params.limit = count; - LexInterval li; - if (!ParseLexBound(min_s, &li.first) || !ParseLexBound(max_s, &li.second)) { - return (*cntx)->SendError(kLexRangeErr); - } - - ZRangeSpec range_spec; - range_spec.params.offset = offset; - range_spec.params.limit = count; - range_spec.interval = li; - - auto cb = [&](Transaction* t, EngineShard* shard) { - return OpRange(range_spec, t->GetOpArgs(shard), key); - }; - - OpResult result = cntx->transaction->ScheduleSingleHopT(std::move(cb)); - OutputScoredArrayResult(result, range_spec.params, cntx); + ZRangeGeneric(args, range_params, cntx); } void ZSetFamily::ZRangeByScore(CmdArgList args, ConnectionContext* cntx) { - string_view key = ArgS(args, 1); - string_view min_s = ArgS(args, 2); - string_view max_s = ArgS(args, 3); - - RangeParams range_params; - args.remove_prefix(4); - - if (!ParseRangeByScoreParams(args, &range_params)) { - return (*cntx)->SendError(kSyntaxErr); - } - - ZRangeByScoreInternal(key, min_s, max_s, range_params, cntx); + ZRangeByScoreInternal(std::move(args), false, cntx); } void ZSetFamily::ZRemRangeByRank(CmdArgList args, ConnectionContext* cntx) { @@ -1492,23 +1514,14 @@ void ZSetFamily::ZUnionStore(CmdArgList args, ConnectionContext* cntx) { (*cntx)->SendLong(smvec.size()); } -void ZSetFamily::ZRangeByScoreInternal(string_view key, string_view min_s, string_view max_s, - const RangeParams& params, ConnectionContext* cntx) { - ZRangeSpec range_spec; - range_spec.params = params; - - ScoreInterval si; - if (!ParseBound(min_s, &si.first) || !ParseBound(max_s, &si.second)) { - return (*cntx)->SendError(kFloatRangeErr); +void ZSetFamily::ZRangeByScoreInternal(CmdArgList args, bool reverse, ConnectionContext* cntx) { + RangeParams range_params; + range_params.interval_type = RangeParams::INTERVALTYPE::SCORE; + range_params.reverse = reverse; + if (!ParseRangeByScoreParams(args.subspan(4), &range_params)) { + return (*cntx)->SendError(kSyntaxErr); } - range_spec.interval = si; - - auto cb = [&](Transaction* t, EngineShard* shard) { - return OpRange(range_spec, t->GetOpArgs(shard), key); - }; - - OpResult result = cntx->transaction->ScheduleSingleHopT(std::move(cb)); - OutputScoredArrayResult(result, params, cntx); + ZRangeGeneric(args, range_params, cntx); } void ZSetFamily::OutputScoredArrayResult(const OpResult& result, @@ -1545,44 +1558,42 @@ void ZSetFamily::ZRemRangeGeneric(string_view key, const ZRangeSpec& range_spec, } } -void ZSetFamily::ZRangeGeneric(CmdArgList args, bool reverse, ConnectionContext* cntx) { +void ZSetFamily::ZRangeGeneric(CmdArgList args, RangeParams range_params, ConnectionContext* cntx) { string_view key = ArgS(args, 1); string_view min_s = ArgS(args, 2); string_view max_s = ArgS(args, 3); - bool parse_score = false; - RangeParams range_params; - range_params.reverse = reverse; - - for (size_t i = 4; i < args.size(); ++i) { - ToUpper(&args[i]); + ZRangeSpec range_spec; + range_spec.params = range_params; - string_view cur_arg = ArgS(args, i); - if (!reverse && cur_arg == "BYSCORE") { - parse_score = true; - } else if (cur_arg == "WITHSCORES") { - range_params.with_scores = true; - } else { - return cntx->reply_builder()->SendError(absl::StrCat("unsupported option ", cur_arg)); + switch (range_params.interval_type) { + case RangeParams::INTERVALTYPE::SCORE: { + ScoreInterval si; + if (!ParseBound(min_s, &si.first) || !ParseBound(max_s, &si.second)) { + return (*cntx)->SendError(kFloatRangeErr); + } + range_spec.interval = si; + break; + } + case RangeParams::INTERVALTYPE::LEX: { + LexInterval li; + if (!ParseLexBound(min_s, &li.first) || !ParseLexBound(max_s, &li.second)) { + return (*cntx)->SendError(kLexRangeErr); + } + range_spec.interval = li; + break; + } + case RangeParams::INTERVALTYPE::RANK: { + IndexInterval ii; + if (!SimpleAtoi(min_s, &ii.first) || !SimpleAtoi(max_s, &ii.second)) { + (*cntx)->SendError(kInvalidIntErr); + return; + } + range_spec.interval = ii; + break; } } - if (parse_score) { - ZRangeByScoreInternal(key, min_s, max_s, range_params, cntx); - return; - } - - IndexInterval ii; - - if (!SimpleAtoi(min_s, &ii.first) || !SimpleAtoi(max_s, &ii.second)) { - (*cntx)->SendError(kInvalidIntErr); - return; - } - - ZRangeSpec range_spec; - range_spec.params = range_params; - range_spec.interval = ii; - auto cb = [&](Transaction* t, EngineShard* shard) { return OpRange(range_spec, t->GetOpArgs(shard), key); }; @@ -1758,7 +1769,8 @@ OpResult ZSetFamily::OpScore(const OpArgs& op_args, string_view key, str return score; } -OpResult ZSetFamily::OpMScore(const OpArgs& op_args, string_view key, ArgSlice members) { +OpResult ZSetFamily::OpMScore(const OpArgs& op_args, string_view key, + ArgSlice members) { OpResult res_it = op_args.shard->db_slice().Find(op_args.db_cntx, key, OBJ_ZSET); if (!res_it) return res_it.status(); @@ -1784,7 +1796,8 @@ OpResult ZSetFamily::OpMScore(const OpArgs& op_args, return scores; } -auto ZSetFamily::OpPopCount(const ZRangeSpec& range_spec, const OpArgs& op_args, string_view key) -> OpResult { +auto ZSetFamily::OpPopCount(const ZRangeSpec& range_spec, const OpArgs& op_args, string_view key) + -> OpResult { auto& db_slice = op_args.shard->db_slice(); OpResult res_it = db_slice.Find(op_args.db_cntx, key, OBJ_ZSET); if (!res_it) @@ -2020,6 +2033,7 @@ void ZSetFamily::Register(CommandRegistry* registry) { << CI{"ZREMRANGEBYSCORE", CO::WRITE, 4, 1, 1, 1}.HFUNC(ZRemRangeByScore) << CI{"ZREMRANGEBYLEX", CO::WRITE, 4, 1, 1, 1}.HFUNC(ZRemRangeByLex) << CI{"ZREVRANGE", CO::READONLY, -4, 1, 1, 1}.HFUNC(ZRevRange) + << CI{"ZREVRANGEBYLEX", CO::READONLY, -4, 1, 1, 1}.HFUNC(ZRevRangeByLex) << CI{"ZREVRANGEBYSCORE", CO::READONLY, -4, 1, 1, 1}.HFUNC(ZRevRangeByScore) << CI{"ZREVRANK", CO::READONLY | CO::FAST, 3, 1, 1, 1}.HFUNC(ZRevRank) << CI{"ZSCAN", CO::READONLY, -3, 1, 1, 1}.HFUNC(ZScan) diff --git a/src/server/zset_family.h b/src/server/zset_family.h index b39ad6bfb674..7433c962a8f5 100644 --- a/src/server/zset_family.h +++ b/src/server/zset_family.h @@ -29,7 +29,7 @@ class ZSetFamily { struct LexBound { std::string_view val; - enum Type {PLUS_INF, MINUS_INF, OPEN, CLOSED} type = CLOSED; + enum Type { PLUS_INF, MINUS_INF, OPEN, CLOSED } type = CLOSED; }; using LexInterval = std::pair; @@ -41,6 +41,7 @@ class ZSetFamily { uint32_t limit = UINT32_MAX; bool with_scores = false; bool reverse = false; + enum INTERVALTYPE { LEX, RANK, SCORE } interval_type = RANK; }; struct ZRangeSpec { @@ -68,6 +69,8 @@ class ZSetFamily { static void ZScore(CmdArgList args, ConnectionContext* cntx); static void ZMScore(CmdArgList args, ConnectionContext* cntx); static void ZRangeByLex(CmdArgList args, ConnectionContext* cntx); + static void ZRevRangeByLex(CmdArgList args, ConnectionContext* cntx); + static void ZRangeByLexInternal(CmdArgList args, bool reverse, ConnectionContext* cntx); static void ZRangeByScore(CmdArgList args, ConnectionContext* cntx); static void ZRemRangeByRank(CmdArgList args, ConnectionContext* cntx); static void ZRemRangeByScore(CmdArgList args, ConnectionContext* cntx); @@ -78,14 +81,12 @@ class ZSetFamily { static void ZScan(CmdArgList args, ConnectionContext* cntx); static void ZUnionStore(CmdArgList args, ConnectionContext* cntx); - static void ZRangeByScoreInternal(std::string_view key, std::string_view min_s, - std::string_view max_s, const RangeParams& params, - ConnectionContext* cntx); + static void ZRangeByScoreInternal(CmdArgList args, bool reverse, ConnectionContext* cntx); static void OutputScoredArrayResult(const OpResult& arr, const RangeParams& params, ConnectionContext* cntx); static void ZRemRangeGeneric(std::string_view key, const ZRangeSpec& range_spec, ConnectionContext* cntx); - static void ZRangeGeneric(CmdArgList args, bool reverse, ConnectionContext* cntx); + static void ZRangeGeneric(CmdArgList args, RangeParams range_params, ConnectionContext* cntx); static void ZRankGeneric(CmdArgList args, bool reverse, ConnectionContext* cntx); static bool ParseRangeByScoreParams(CmdArgList args, RangeParams* params); static void ZPopMinMax(CmdArgList args, bool reverse, ConnectionContext* cntx); @@ -96,7 +97,7 @@ class ZSetFamily { std::string_view member); using MScoreResponse = std::vector>; static OpResult OpMScore(const OpArgs& op_args, std::string_view key, - ArgSlice members); + ArgSlice members); static OpResult OpPopCount(const ZRangeSpec& range_spec, const OpArgs& op_args, std::string_view key); static OpResult OpRange(const ZRangeSpec& range_spec, const OpArgs& op_args, @@ -112,7 +113,6 @@ class ZSetFamily { static OpResult OpLexCount(const OpArgs& op_args, std::string_view key, const LexInterval& interval); - }; } // namespace dfly diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc index 33b1ec125adf..39e8331457ab 100644 --- a/src/server/zset_family_test.cc +++ b/src/server/zset_family_test.cc @@ -142,6 +142,69 @@ TEST_F(ZSetFamilyTest, ByLex) { ASSERT_THAT(resp.GetVec(), ElementsAre("alpha", "bar", "cool", "down", "elephant", "foo")); } +TEST_F(ZSetFamilyTest, ZRevRangeByLex) { + Run({ + "zadd", "key", "0", "alpha", "0", "bar", "0", "cool", "0", "down", + "0", "elephant", "0", "foo", "0", "great", "0", "hill", "0", "omega", + }); + + auto resp = Run({"zrevrangebylex", "key", "[cool", "-"}); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + EXPECT_THAT(resp.GetVec(), ElementsAre("cool", "bar", "alpha")); + + EXPECT_EQ(3, CheckedInt({"ZLEXCOUNT", "key", "(foo", "+"})); + EXPECT_EQ(3, CheckedInt({"ZREMRANGEBYLEX", "key", "(foo", "+"})); + + resp = Run({"zrevrangebylex", "key", "+", "[a"}); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + ASSERT_THAT(resp.GetVec(), ElementsAre("foo", "elephant", "down", "cool", "bar", "alpha")); +} + +TEST_F(ZSetFamilyTest, ZRange) { + Run({"zadd", "key", "0", "a", "1", "d", "1", "b", "2", "c", "4", "e"}); + + auto resp = Run({"zrange", "key", "0", "2"}); + ASSERT_THAT(resp, ArrLen(3)); + ASSERT_THAT(resp.GetVec(), ElementsAre("a", "b", "d")); + + resp = Run({"zrange", "key", "1", "3", "WITHSCORES"}); + ASSERT_THAT(resp, ArrLen(6)); + ASSERT_THAT(resp.GetVec(), ElementsAre("b", "1", "d", "1", "c", "2")); + + resp = Run({"zrange", "key", "1", "3", "WITHSCORES", "REV"}); + ASSERT_THAT(resp, ArrLen(6)); + ASSERT_THAT(resp.GetVec(), ElementsAre("c", "2", "d", "1", "b", "1")); + + resp = Run({"zrange", "key", "(1", "4", "BYSCORE", "WITHSCORES"}); + ASSERT_THAT(resp, ArrLen(4)); + ASSERT_THAT(resp.GetVec(), ElementsAre("c", "2", "e", "4")); + + resp = Run({"zrange", "key", "-", "d", "BYLEX", "BYSCORE"}); + EXPECT_THAT(resp, ErrArg("BYSCORE and BYLEX options are not compatible")); + + Run({"zremrangebyscore", "key", "0", "4"}); + + Run({ + "zadd", "key", "0", "alpha", "0", "bar", "0", "cool", "0", "down", + "0", "elephant", "0", "foo", "0", "great", "0", "hill", "0", "omega", + }); + resp = Run({"zrange", "key", "-", "[cool", "BYLEX"}); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + EXPECT_THAT(resp.GetVec(), ElementsAre("alpha", "bar", "cool")); + + resp = Run({"zrange", "key", "[cool", "-", "REV", "BYLEX"}); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + EXPECT_THAT(resp.GetVec(), ElementsAre("cool", "bar", "alpha")); + + resp = Run({"zrange", "key", "+", "[cool", "REV", "BYLEX", "LIMIT", "2", "2"}); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + EXPECT_THAT(resp.GetVec(), ElementsAre("great", "foo")); + + + resp = Run({"zrange", "key", "+", "[cool", "BYLEX", "LIMIT", "2", "2", "REV"}); + EXPECT_THAT(resp, ErrArg("syntax error")); +} + TEST_F(ZSetFamilyTest, ZRevRange) { Run({"zadd", "key", "-inf", "a", "1", "b", "2", "c"}); auto resp = Run({"zrevrangebyscore", "key", "2", "-inf"}); From 97468bdb8aaf62058961217c7d54991d6ae23f1a Mon Sep 17 00:00:00 2001 From: Redhal Date: Wed, 2 Nov 2022 09:55:13 +0100 Subject: [PATCH 2/5] Fixed handling of LIMIT args --- src/server/zset_family.cc | 2 +- src/server/zset_family_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 01cbadcf806e..604639a2c94d 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -1225,7 +1225,7 @@ void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { } else if (cur_arg == "WITHSCORES") { range_params.with_scores = true; } else if (cur_arg == "LIMIT") { - if (i + 3 != args.size()) { + if (i + 3 > args.size()) { return (*cntx)->SendError(kSyntaxErr); } string_view os = ArgS(args, i + 1); diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc index 39e8331457ab..b199087b3ed3 100644 --- a/src/server/zset_family_test.cc +++ b/src/server/zset_family_test.cc @@ -200,9 +200,9 @@ TEST_F(ZSetFamilyTest, ZRange) { ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); EXPECT_THAT(resp.GetVec(), ElementsAre("great", "foo")); - resp = Run({"zrange", "key", "+", "[cool", "BYLEX", "LIMIT", "2", "2", "REV"}); - EXPECT_THAT(resp, ErrArg("syntax error")); + ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); + EXPECT_THAT(resp.GetVec(), ElementsAre("great", "foo")); } TEST_F(ZSetFamilyTest, ZRevRange) { From 2e4e39e3f46a4a3308f46db32bde97db560be0d0 Mon Sep 17 00:00:00 2001 From: Redhal Date: Wed, 2 Nov 2022 10:14:55 +0100 Subject: [PATCH 3/5] limit args increment fix --- src/server/zset_family.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 604639a2c94d..075cade64bd5 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -1233,7 +1233,7 @@ void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { if (!SimpleAtoi(os, &range_params.offset) || !SimpleAtoi(cs, &range_params.limit)) { return (*cntx)->SendError(kInvalidIntErr); } - i += 3; + i += 2; } else { return cntx->reply_builder()->SendError(absl::StrCat("unsupported option ", cur_arg)); } From 3321d16aa37f0bb79e124ab35e4d9797efa7a2c3 Mon Sep 17 00:00:00 2001 From: Redhal Date: Wed, 2 Nov 2022 12:41:06 +0100 Subject: [PATCH 4/5] Renamed intervaltype enum --- src/server/zset_family.cc | 21 +++++++++------------ src/server/zset_family.h | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 075cade64bd5..0a83f0f0427d 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -1203,7 +1203,6 @@ void ZSetFamily::ZLexCount(CmdArgList args, ConnectionContext* cntx) { } void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { - RangeParams::INTERVALTYPE interval_type = RangeParams::INTERVALTYPE::RANK; RangeParams range_params; for (size_t i = 4; i < args.size(); ++i) { @@ -1211,15 +1210,15 @@ void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { string_view cur_arg = ArgS(args, i); if (cur_arg == "BYSCORE") { - if (interval_type == RangeParams::INTERVALTYPE::LEX) { + if (range_params.interval_type == RangeParams::IntervalType::LEX) { return (*cntx)->SendError("BYSCORE and BYLEX options are not compatible"); } - interval_type = RangeParams::INTERVALTYPE::SCORE; + range_params.interval_type = RangeParams::IntervalType::SCORE; } else if (cur_arg == "BYLEX") { - if (interval_type == RangeParams::INTERVALTYPE::SCORE) { + if (range_params.interval_type == RangeParams::IntervalType::SCORE) { return (*cntx)->SendError("BYSCORE and BYLEX options are not compatible"); } - interval_type = RangeParams::INTERVALTYPE::LEX; + range_params.interval_type = RangeParams::IntervalType::LEX; } else if (cur_arg == "REV") { range_params.reverse = true; } else if (cur_arg == "WITHSCORES") { @@ -1238,8 +1237,6 @@ void ZSetFamily::ZRange(CmdArgList args, ConnectionContext* cntx) { return cntx->reply_builder()->SendError(absl::StrCat("unsupported option ", cur_arg)); } } - - range_params.interval_type = interval_type; ZRangeGeneric(std::move(args), range_params, cntx); } @@ -1285,7 +1282,7 @@ void ZSetFamily::ZRangeByLexInternal(CmdArgList args, bool reverse, ConnectionCo uint32_t count = kuint32max; RangeParams range_params; - range_params.interval_type = RangeParams::INTERVALTYPE::LEX; + range_params.interval_type = RangeParams::IntervalType::LEX; range_params.reverse = reverse; if (args.size() > 4) { @@ -1516,7 +1513,7 @@ void ZSetFamily::ZUnionStore(CmdArgList args, ConnectionContext* cntx) { void ZSetFamily::ZRangeByScoreInternal(CmdArgList args, bool reverse, ConnectionContext* cntx) { RangeParams range_params; - range_params.interval_type = RangeParams::INTERVALTYPE::SCORE; + range_params.interval_type = RangeParams::IntervalType::SCORE; range_params.reverse = reverse; if (!ParseRangeByScoreParams(args.subspan(4), &range_params)) { return (*cntx)->SendError(kSyntaxErr); @@ -1567,7 +1564,7 @@ void ZSetFamily::ZRangeGeneric(CmdArgList args, RangeParams range_params, Connec range_spec.params = range_params; switch (range_params.interval_type) { - case RangeParams::INTERVALTYPE::SCORE: { + case RangeParams::IntervalType::SCORE: { ScoreInterval si; if (!ParseBound(min_s, &si.first) || !ParseBound(max_s, &si.second)) { return (*cntx)->SendError(kFloatRangeErr); @@ -1575,7 +1572,7 @@ void ZSetFamily::ZRangeGeneric(CmdArgList args, RangeParams range_params, Connec range_spec.interval = si; break; } - case RangeParams::INTERVALTYPE::LEX: { + case RangeParams::IntervalType::LEX: { LexInterval li; if (!ParseLexBound(min_s, &li.first) || !ParseLexBound(max_s, &li.second)) { return (*cntx)->SendError(kLexRangeErr); @@ -1583,7 +1580,7 @@ void ZSetFamily::ZRangeGeneric(CmdArgList args, RangeParams range_params, Connec range_spec.interval = li; break; } - case RangeParams::INTERVALTYPE::RANK: { + case RangeParams::IntervalType::RANK: { IndexInterval ii; if (!SimpleAtoi(min_s, &ii.first) || !SimpleAtoi(max_s, &ii.second)) { (*cntx)->SendError(kInvalidIntErr); diff --git a/src/server/zset_family.h b/src/server/zset_family.h index 7433c962a8f5..56e31438d888 100644 --- a/src/server/zset_family.h +++ b/src/server/zset_family.h @@ -41,7 +41,7 @@ class ZSetFamily { uint32_t limit = UINT32_MAX; bool with_scores = false; bool reverse = false; - enum INTERVALTYPE { LEX, RANK, SCORE } interval_type = RANK; + enum IntervalType { LEX, RANK, SCORE } interval_type = RANK; }; struct ZRangeSpec { From 954217cf32884fa8866f652a9b04fcd4ff47e3b0 Mon Sep 17 00:00:00 2001 From: Redhal Date: Thu, 3 Nov 2022 11:47:31 +0100 Subject: [PATCH 5/5] fix arg order in zrangebyscore --- src/server/zset_family.cc | 4 ++-- src/server/zset_family_test.cc | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 0a83f0f0427d..92f24e0b1215 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -1625,7 +1625,7 @@ bool ZSetFamily::ParseRangeByScoreParams(CmdArgList args, RangeParams* params) { if (cur_arg == "WITHSCORES") { params->with_scores = true; } else if (cur_arg == "LIMIT") { - if (i + 3 != args.size()) + if (i + 3 > args.size()) return false; string_view os = ArgS(args, i + 1); @@ -1633,7 +1633,7 @@ bool ZSetFamily::ParseRangeByScoreParams(CmdArgList args, RangeParams* params) { if (!SimpleAtoi(os, ¶ms->offset) || !SimpleAtoi(cs, ¶ms->limit)) return false; - i += 3; + i += 2; } else { return false; } diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc index b199087b3ed3..b43048914048 100644 --- a/src/server/zset_family_test.cc +++ b/src/server/zset_family_test.cc @@ -78,7 +78,15 @@ TEST_F(ZSetFamilyTest, ZRangeRank) { EXPECT_THAT(Run({"zrangebyscore", "x", "0", "(1.1"}), ArrLen(0)); EXPECT_THAT(Run({"zrangebyscore", "x", "-inf", "1.1", "limit", "0", "10"}), "a"); - auto resp = Run({"zrevrangebyscore", "x", "+inf", "-inf", "limit", "0", "5"}); + auto resp = Run({"zrangebyscore", "x", "-inf", "1.1", "limit", "0", "10", "WITHSCORES"}); + ASSERT_THAT(resp, ArrLen(2)); + ASSERT_THAT(resp.GetVec(), ElementsAre("a", "1.1")); + + resp = Run({"zrangebyscore", "x", "-inf", "1.1", "WITHSCORES", "limit", "0", "10"}); + ASSERT_THAT(resp, ArrLen(2)); + ASSERT_THAT(resp.GetVec(), ElementsAre("a", "1.1")); + + resp = Run({"zrevrangebyscore", "x", "+inf", "-inf", "limit", "0", "5"}); ASSERT_THAT(resp, ArgType(RespExpr::ARRAY)); ASSERT_THAT(resp.GetVec(), ElementsAre("b", "a"));