From 492cb010b17b8a44e5b7fbf3d986a3c4aaaaf966 Mon Sep 17 00:00:00 2001 From: Binbin Date: Fri, 26 Jan 2024 18:36:32 +0800 Subject: [PATCH] code review: change to use CommandKeyRangeVecGen --- src/commands/cmd_geo.cc | 20 +++++++++--- src/commands/cmd_list.cc | 2 -- src/commands/cmd_zset.cc | 12 ++++---- src/commands/commander.cc | 10 ------ src/commands/commander.h | 37 ++++------------------- tests/gocase/unit/command/command_test.go | 24 +++++++++++++-- 6 files changed, 49 insertions(+), 56 deletions(-) diff --git a/src/commands/cmd_geo.cc b/src/commands/cmd_geo.cc index 7e924a796f5..f1fd5e62666 100644 --- a/src/commands/cmd_geo.cc +++ b/src/commands/cmd_geo.cc @@ -344,7 +344,7 @@ class CommandGeoRadius : public CommandGeoBase { return redis::Array(list); } - static CommandKeyRange Range(const std::vector &args) { + static std::vector Range(const std::vector &args) { int store_key = 0; // Check for the presence of the stored key in the command args. @@ -358,7 +358,10 @@ class CommandGeoRadius : public CommandGeoBase { } } - return {1, 1, 1, store_key}; + if (store_key > 0) { + return {{1, 1, 1}, {store_key, store_key, 1}}; + } + return {{1, 1, 1}}; } protected: @@ -622,6 +625,10 @@ class CommandGeoSearchStore : public CommandGeoSearch { return Status::OK(); } + static std::vector Range(const std::vector &args) { + return {{1, 1, 1}, {2, 2, 1}}; + } + private: bool store_distance_ = false; std::string store_key_; @@ -665,7 +672,7 @@ class CommandGeoRadiusByMember : public CommandGeoRadius { return Status::OK(); } - static CommandKeyRange Range(const std::vector &args) { + static std::vector Range(const std::vector &args) { int store_key = 0; // Check for the presence of the stored key in the command args. @@ -679,7 +686,10 @@ class CommandGeoRadiusByMember : public CommandGeoRadius { } } - return {1, 1, 1, store_key}; + if (store_key > 0) { + return {{1, 1, 1}, {store_key, store_key, 1}}; + } + return {{1, 1, 1}}; } }; @@ -703,6 +713,6 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr("geoadd", -5, "write", 1, 1, MakeCmdAttr("georadius_ro", -6, "read-only", 1, 1, 1), MakeCmdAttr("georadiusbymember_ro", -5, "read-only", 1, 1, 1), MakeCmdAttr("geosearch", -7, "read-only", 1, 1, 1), - MakeCmdAttr("geosearchstore", -8, "write", 2, 2, 1, 1)) + MakeCmdAttr("geosearchstore", -8, "write", CommandGeoSearchStore::Range)) } // namespace redis diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index ce20a60e385..726d2a70889 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -225,7 +225,6 @@ class CommandLMPop : public Commander { // This parsing would always succeed as this cmd has been parsed before. auto num_key = *ParseInt(args[1], 10); range.last_key = range.first_key + num_key - 1; - range.store_key = 0; return range; }; @@ -447,7 +446,6 @@ class CommandBLMPop : public BlockingCommander { // This parsing would always succeed as this cmd has been parsed before. auto num_key = *ParseInt(args[2], 10); range.last_key = range.first_key + num_key - 1; - range.store_key = 0; return range; }; diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index 7301ec5b027..d78fe54eed8 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -1223,9 +1223,9 @@ class CommandZUnionStore : public Commander { return Status::OK(); } - static CommandKeyRange Range(const std::vector &args) { + static std::vector Range(const std::vector &args) { int num_key = *ParseInt(args[2], 10); - return {3, 2 + num_key, 1, 1}; + return {{1, 1, 1}, {3, 2 + num_key, 1}}; } protected: @@ -1250,9 +1250,9 @@ class CommandZInterStore : public CommandZUnionStore { return Status::OK(); } - static CommandKeyRange Range(const std::vector &args) { + static std::vector Range(const std::vector &args) { int num_key = *ParseInt(args[2], 10); - return {3, 2 + num_key, 1, 1}; + return {{1, 1, 1}, {3, 2 + num_key, 1}}; } }; @@ -1506,9 +1506,9 @@ class CommandZDiffStore : public Commander { return Status::OK(); } - static CommandKeyRange Range(const std::vector &args) { + static std::vector Range(const std::vector &args) { int num_key = *ParseInt(args[2], 10); - return {3, 2 + num_key, 1, 1}; + return {{1, 1, 1}, {3, 2 + num_key, 1}}; } protected: diff --git a/src/commands/commander.cc b/src/commands/commander.cc index e8170985592..6ac10581606 100644 --- a/src/commands/commander.cc +++ b/src/commands/commander.cc @@ -81,19 +81,9 @@ void DumpKeyRange(std::vector &keys_index, int argc, const CommandKeyRange auto last = range.last_key; if (last < 0) last = argc + last; - // The index of store key is before the first key - if (range.store_key > 0 && range.store_key < range.first_key) { - keys_index.emplace_back(range.store_key); - } - for (int i = range.first_key; i <= last; i += range.key_step) { keys_index.emplace_back(i); } - - // The index of store key is after the last key - if (range.store_key > 0 && range.store_key > last) { - keys_index.emplace_back(range.store_key); - } } Status CommandTable::GetKeysFromCommand(const CommandAttributes *attributes, const std::vector &cmd_tokens, diff --git a/src/commands/commander.h b/src/commands/commander.h index 2161daed73d..3330337c746 100644 --- a/src/commands/commander.h +++ b/src/commands/commander.h @@ -93,22 +93,18 @@ class CommanderWithParseMove : Commander { using CommanderFactory = std::function()>; struct CommandKeyRange { - // index of the first key (non-store key) in command tokens + // index of the first key in command tokens // 0 stands for no key, since the first index of command arguments is command name int first_key; - // index of the last key (non-store key) in command tokens + // index of the last key in command tokens // in normal one-key commands, first key and last key index are both 1 // -n stands for the n-th last index of the sequence, i.e. args.size() - n int last_key; - // step length of key (non-store key) position + // step length of key position // e.g. key step 2 means "key other key other ..." sequence int key_step; - - // index of the store key in command tokens - // 0 stands for no store key, since the first index of command arguments is command name - int store_key; }; using CommandKeyRangeGen = std::function &)>; @@ -201,7 +197,7 @@ auto MakeCmdAttr(const std::string &name, int arity, const std::string &descript description, ParseCommandFlags(description, name), flag_gen, - {first_key, last_key, key_step, 0}, + {first_key, last_key, key_step}, {}, {}, []() -> std::unique_ptr { return std::unique_ptr(new T()); }}; @@ -214,27 +210,6 @@ auto MakeCmdAttr(const std::string &name, int arity, const std::string &descript return attr; } -template -auto MakeCmdAttr(const std::string &name, int arity, const std::string &description, int first_key, int last_key, - int key_step, int store_key, const AdditionalFlagGen &flag_gen = {}) { - CommandAttributes attr{name, - arity, - description, - ParseCommandFlags(description, name), - flag_gen, - {first_key, last_key, key_step, store_key}, - {}, - {}, - []() -> std::unique_ptr { return std::unique_ptr(new T()); }}; - - if ((first_key > 0 && key_step <= 0) || (first_key > 0 && last_key >= 0 && last_key < first_key) || (store_key < 0)) { - std::cout << fmt::format("Encountered invalid key range in command {}", name) << std::endl; - std::abort(); - } - - return attr; -} - template auto MakeCmdAttr(const std::string &name, int arity, const std::string &description, const CommandKeyRangeGen &gen, const AdditionalFlagGen &flag_gen = {}) { @@ -243,7 +218,7 @@ auto MakeCmdAttr(const std::string &name, int arity, const std::string &descript description, ParseCommandFlags(description, name), flag_gen, - {-1, 0, 0, 0}, + {-1, 0, 0}, gen, {}, []() -> std::unique_ptr { return std::unique_ptr(new T()); }}; @@ -259,7 +234,7 @@ auto MakeCmdAttr(const std::string &name, int arity, const std::string &descript description, ParseCommandFlags(description, name), flag_gen, - {-2, 0, 0, 0}, + {-2, 0, 0}, {}, vec_gen, []() -> std::unique_ptr { return std::unique_ptr(new T()); }}; diff --git a/tests/gocase/unit/command/command_test.go b/tests/gocase/unit/command/command_test.go index 8da82510a1b..51be1347fd7 100644 --- a/tests/gocase/unit/command/command_test.go +++ b/tests/gocase/unit/command/command_test.go @@ -178,13 +178,22 @@ func TestCommand(t *testing.T) { }) t.Run("COMMAND GETKEYS GEORADIUS", func(t *testing.T) { - r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUS", "src", "1", "1", "1", "km", "store", "dst") + // non-store + r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUS", "src", "1", "1", "1", "km") vs, err := r.Slice() require.NoError(t, err) + require.Len(t, vs, 1) + require.Equal(t, "src", vs[0]) + + // store + r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUS", "src", "1", "1", "1", "km", "store", "dst") + vs, err = r.Slice() + require.NoError(t, err) require.Len(t, vs, 2) require.Equal(t, "src", vs[0]) require.Equal(t, "dst", vs[1]) + // storedist r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUS", "src", "1", "1", "1", "km", "storedist", "dst") vs, err = r.Slice() require.NoError(t, err) @@ -192,6 +201,7 @@ func TestCommand(t *testing.T) { require.Equal(t, "src", vs[0]) require.Equal(t, "dst", vs[1]) + // store + storedist r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUS", "src", "1", "1", "1", "km", "store", "dst1", "storedist", "dst2") vs, err = r.Slice() require.NoError(t, err) @@ -201,13 +211,22 @@ func TestCommand(t *testing.T) { }) t.Run("COMMAND GETKEYS GEORADIUSBYMEMBER", func(t *testing.T) { - r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "store", "dst") + // non-store + r := rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m") vs, err := r.Slice() require.NoError(t, err) + require.Len(t, vs, 1) + require.Equal(t, "src", vs[0]) + + // store + r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "store", "dst") + vs, err = r.Slice() + require.NoError(t, err) require.Len(t, vs, 2) require.Equal(t, "src", vs[0]) require.Equal(t, "dst", vs[1]) + // storedist r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "storedist", "dst") vs, err = r.Slice() require.NoError(t, err) @@ -215,6 +234,7 @@ func TestCommand(t *testing.T) { require.Equal(t, "src", vs[0]) require.Equal(t, "dst", vs[1]) + // store + storedist r = rdb.Do(ctx, "COMMAND", "GETKEYS", "GEORADIUSBYMEMBER", "src", "member", "radius", "m", "store", "dst1", "storedist", "dst2") vs, err = r.Slice() require.NoError(t, err)