Skip to content

Commit

Permalink
code review: change to use CommandKeyRangeVecGen
Browse files Browse the repository at this point in the history
  • Loading branch information
@enjoy-binbin
enjoy-binbin committed Jan 26, 2024
1 parent 611cd77 commit 492cb01
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 56 deletions.
20 changes: 15 additions & 5 deletions src/commands/cmd_geo.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ class CommandGeoRadius : public CommandGeoBase {
return redis::Array(list);
}

static CommandKeyRange Range(const std::vector<std::string> &args) {
static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
int store_key = 0;

// Check for the presence of the stored key in the command args.
Expand All @@ -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:
Expand Down Expand Up @@ -622,6 +625,10 @@ class CommandGeoSearchStore : public CommandGeoSearch {
return Status::OK();
}

static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
return {{1, 1, 1}, {2, 2, 1}};
}

private:
bool store_distance_ = false;
std::string store_key_;
Expand Down Expand Up @@ -665,7 +672,7 @@ class CommandGeoRadiusByMember : public CommandGeoRadius {
return Status::OK();
}

static CommandKeyRange Range(const std::vector<std::string> &args) {
static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
int store_key = 0;

// Check for the presence of the stored key in the command args.
Expand All @@ -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}};
}
};

Expand All @@ -703,6 +713,6 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandGeoAdd>("geoadd", -5, "write", 1, 1,
MakeCmdAttr<CommandGeoRadiusReadonly>("georadius_ro", -6, "read-only", 1, 1, 1),
MakeCmdAttr<CommandGeoRadiusByMemberReadonly>("georadiusbymember_ro", -5, "read-only", 1, 1, 1),
MakeCmdAttr<CommandGeoSearch>("geosearch", -7, "read-only", 1, 1, 1),
MakeCmdAttr<CommandGeoSearchStore>("geosearchstore", -8, "write", 2, 2, 1, 1))
MakeCmdAttr<CommandGeoSearchStore>("geosearchstore", -8, "write", CommandGeoSearchStore::Range))

} // namespace redis
2 changes: 0 additions & 2 deletions src/commands/cmd_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ class CommandLMPop : public Commander {
// This parsing would always succeed as this cmd has been parsed before.
auto num_key = *ParseInt<int32_t>(args[1], 10);
range.last_key = range.first_key + num_key - 1;
range.store_key = 0;
return range;
};

Expand Down Expand Up @@ -447,7 +446,6 @@ class CommandBLMPop : public BlockingCommander {
// This parsing would always succeed as this cmd has been parsed before.
auto num_key = *ParseInt<int32_t>(args[2], 10);
range.last_key = range.first_key + num_key - 1;
range.store_key = 0;
return range;
};

Expand Down
12 changes: 6 additions & 6 deletions src/commands/cmd_zset.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1223,9 +1223,9 @@ class CommandZUnionStore : public Commander {
return Status::OK();
}

static CommandKeyRange Range(const std::vector<std::string> &args) {
static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
int num_key = *ParseInt<int>(args[2], 10);
return {3, 2 + num_key, 1, 1};
return {{1, 1, 1}, {3, 2 + num_key, 1}};
}

protected:
Expand All @@ -1250,9 +1250,9 @@ class CommandZInterStore : public CommandZUnionStore {
return Status::OK();
}

static CommandKeyRange Range(const std::vector<std::string> &args) {
static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
int num_key = *ParseInt<int>(args[2], 10);
return {3, 2 + num_key, 1, 1};
return {{1, 1, 1}, {3, 2 + num_key, 1}};
}
};

Expand Down Expand Up @@ -1506,9 +1506,9 @@ class CommandZDiffStore : public Commander {
return Status::OK();
}

static CommandKeyRange Range(const std::vector<std::string> &args) {
static std::vector<CommandKeyRange> Range(const std::vector<std::string> &args) {
int num_key = *ParseInt<int>(args[2], 10);
return {3, 2 + num_key, 1, 1};
return {{1, 1, 1}, {3, 2 + num_key, 1}};
}

protected:
Expand Down
10 changes: 0 additions & 10 deletions src/commands/commander.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,9 @@ void DumpKeyRange(std::vector<int> &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<std::string> &cmd_tokens,
Expand Down
37 changes: 6 additions & 31 deletions src/commands/commander.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,18 @@ class CommanderWithParseMove : Commander {
using CommanderFactory = std::function<std::unique_ptr<Commander>()>;

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<CommandKeyRange(const std::vector<std::string> &)>;
Expand Down Expand Up @@ -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<Commander> { return std::unique_ptr<Commander>(new T()); }};
Expand All @@ -214,27 +210,6 @@ auto MakeCmdAttr(const std::string &name, int arity, const std::string &descript
return attr;
}

template <typename T>
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<Commander> { return std::unique_ptr<Commander>(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 <typename T>
auto MakeCmdAttr(const std::string &name, int arity, const std::string &description, const CommandKeyRangeGen &gen,
const AdditionalFlagGen &flag_gen = {}) {
Expand All @@ -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<Commander> { return std::unique_ptr<Commander>(new T()); }};
Expand All @@ -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<Commander> { return std::unique_ptr<Commander>(new T()); }};
Expand Down
24 changes: 22 additions & 2 deletions tests/gocase/unit/command/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,20 +178,30 @@ 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)
require.Len(t, vs, 2)
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)
Expand All @@ -201,20 +211,30 @@ 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)
require.Len(t, vs, 2)
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)
Expand Down

0 comments on commit 492cb01

Please sign in to comment.