Skip to content

Commit 6ecbc92

Browse files
H4R5H1T-007H4R5H1T-007
authored
feat: Added Sort_RO Command. (#5332)
* Added Sort_RO Command.
1 parent acf5f19 commit 6ecbc92

File tree

3 files changed

+126
-14
lines changed

3 files changed

+126
-14
lines changed

‎src/server/generic_family.cc‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1471,7 +1471,7 @@ OpResult<uint32_t> OpStore(const OpArgs& op_args, std::string_view key, Iterator
14711471
return len;
14721472
}
14731473

1474-
void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
1474+
void SortGeneric(CmdArgList args, const CommandContext& cmd_cntx, bool is_read_only) {
14751475
std::string_view key = ArgS(args, 0);
14761476
bool alpha = false;
14771477
bool reversed = false;
@@ -1497,7 +1497,7 @@ void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
14971497
}
14981498
bounds = {offset, limit};
14991499
i += 2;
1500-
} else if (arg == "STORE") {
1500+
} else if (!is_read_only && arg == "STORE") {
15011501
if (i + 1 >= args.size()) {
15021502
return builder->SendError(kSyntaxErr);
15031503
}
@@ -1509,6 +1509,11 @@ void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
15091509
}
15101510
}
15111511

1512+
// Asserting that if is_read_only as true, then store_key should not exist.
1513+
DLOG(INFO) << "is_read_only parameter: " << is_read_only
1514+
<< " and store_key parameter: " << bool(store_key);
1515+
assert(((is_read_only && !bool(store_key)) || !is_read_only));
1516+
15121517
ShardId source_sid = Shard(key, shard_set->size());
15131518
OpResultTyped<SortEntryList> fetch_result;
15141519
auto fetch_cb = [&](Transaction* t, EngineShard* shard) {
@@ -1533,7 +1538,9 @@ void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
15331538
}
15341539

15351540
auto result_type = fetch_result.type();
1536-
auto sort_call = [builder, bounds, reversed, result_type, &store_key, &cmd_cntx](auto& entries) {
1541+
1542+
auto sort_call = [result_type, bounds, reversed, is_read_only, &rb, &store_key,
1543+
&cmd_cntx](auto& entries) {
15371544
using value_t = typename std::decay_t<decltype(entries)>::value_type;
15381545
auto cmp = reversed ? &value_t::greater : &value_t::less;
15391546
if (bounds) {
@@ -1550,9 +1557,15 @@ void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
15501557
end_it = entries.begin() + std::min(bounds->first + bounds->second, entries.size());
15511558
}
15521559

1553-
bool is_set = (result_type == OBJ_SET || result_type == OBJ_ZSET);
1554-
auto* rb = static_cast<RedisReplyBuilder*>(builder);
1555-
if (store_key) {
1560+
if (!bool(store_key)) {
1561+
bool is_set = (result_type == OBJ_SET || result_type == OBJ_ZSET);
1562+
rb->StartCollection(std::distance(start_it, end_it),
1563+
is_set ? RedisReplyBuilder::SET : RedisReplyBuilder::ARRAY);
1564+
1565+
for (auto it = start_it; it != end_it; ++it) {
1566+
rb->SendBulkString(it->key);
1567+
}
1568+
} else {
15561569
ShardId dest_sid = Shard(store_key.value(), shard_set->size());
15571570
OpResult<uint32_t> store_len;
15581571
auto store_callback = [&](Transaction* t, EngineShard* shard) {
@@ -1568,17 +1581,18 @@ void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
15681581
} else {
15691582
rb->SendError(store_len.status());
15701583
}
1571-
} else {
1572-
rb->StartCollection(std::distance(start_it, end_it),
1573-
is_set ? RedisReplyBuilder::SET : RedisReplyBuilder::ARRAY);
1574-
1575-
for (auto it = start_it; it != end_it; ++it) {
1576-
rb->SendBulkString(it->key);
1577-
}
15781584
}
15791585
};
15801586

1581-
std::visit(std::move(sort_call), fetch_result.value());
1587+
std::visit(sort_call, fetch_result.value());
1588+
}
1589+
1590+
void GenericFamily::Sort(CmdArgList args, const CommandContext& cmd_cntx) {
1591+
SortGeneric(args, cmd_cntx, false);
1592+
}
1593+
1594+
void GenericFamily::Sort_RO(CmdArgList args, const CommandContext& cmd_cntx) {
1595+
SortGeneric(args, cmd_cntx, true);
15821596
}
15831597

15841598
void GenericFamily::Restore(CmdArgList args, const CommandContext& cmd_cntx) {
@@ -2004,6 +2018,7 @@ constexpr uint32_t kDump = KEYSPACE | READ | SLOW;
20042018
constexpr uint32_t kUnlink = KEYSPACE | WRITE | FAST;
20052019
constexpr uint32_t kStick = KEYSPACE | WRITE | FAST;
20062020
constexpr uint32_t kSort = WRITE | SET | SORTEDSET | LIST | SLOW | DANGEROUS;
2021+
constexpr uint32_t kSortRO = READ | SET | SORTEDSET | LIST | SLOW | DANGEROUS;
20072022
constexpr uint32_t kMove = KEYSPACE | WRITE | FAST;
20082023
constexpr uint32_t kRestore = KEYSPACE | WRITE | SLOW | DANGEROUS;
20092024
constexpr uint32_t kExpireTime = KEYSPACE | READ | FAST;
@@ -2050,6 +2065,7 @@ void GenericFamily::Register(CommandRegistry* registry) {
20502065
<< CI{"UNLINK", CO::WRITE, -2, 1, -1, acl::kUnlink}.HFUNC(Unlink)
20512066
<< CI{"STICK", CO::WRITE, -2, 1, -1, acl::kStick}.HFUNC(Stick)
20522067
<< CI{"SORT", CO::WRITE, -2, 1, -1, acl::kSort}.HFUNC(Sort)
2068+
<< CI{"SORT_RO", CO::READONLY, -2, 1, 1, acl::kSortRO}.HFUNC(Sort_RO)
20532069
<< CI{"MOVE", CO::WRITE | CO::GLOBAL_TRANS | CO::NO_AUTOJOURNAL, 3, 1, 1, acl::kMove}.HFUNC(
20542070
Move)
20552071
<< CI{"RESTORE", CO::WRITE, -4, 1, 1, acl::kRestore}.HFUNC(Restore)

‎src/server/generic_family.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class GenericFamily {
3939
static void Pexpire(CmdArgList args, const CommandContext& cmd_cntx);
4040
static void Stick(CmdArgList args, const CommandContext& cmd_cntx);
4141
static void Sort(CmdArgList args, const CommandContext& cmd_cntx);
42+
static void Sort_RO(CmdArgList args, const CommandContext& cmd_cntx);
4243
static void Move(CmdArgList args, const CommandContext& cmd_cntx);
4344

4445
static void Rename(CmdArgList args, const CommandContext& cmd_cntx);

‎src/server/generic_family_test.cc‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,6 +869,101 @@ TEST_F(GenericFamilyTest, SortStore) {
869869
ElementsAre("1.2", "2.20", "3.5", "10.1", "200"));
870870
}
871871

872+
TEST_F(GenericFamilyTest, Sort_RO) {
873+
// Test list sort with params
874+
Run({"del", "list-1"});
875+
Run({"lpush", "list-1", "3.5", "1.2", "10.1", "2.20", "200"});
876+
// numeric
877+
ASSERT_THAT(Run({"sort_ro", "list-1"}).GetVec(),
878+
ElementsAre("1.2", "2.20", "3.5", "10.1", "200"));
879+
// string
880+
ASSERT_THAT(Run({"sort_ro", "list-1", "ALPHA"}).GetVec(),
881+
ElementsAre("1.2", "10.1", "2.20", "200", "3.5"));
882+
// desc numeric
883+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC"}).GetVec(),
884+
ElementsAre("200", "10.1", "3.5", "2.20", "1.2"));
885+
// desc strig
886+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC", "ALPHA"}).GetVec(),
887+
ElementsAre("3.5", "200", "2.20", "10.1", "1.2"));
888+
// limits
889+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "0", "5"}).GetVec(),
890+
ElementsAre("1.2", "2.20", "3.5", "10.1", "200"));
891+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "0", "10"}).GetVec(),
892+
ElementsAre("1.2", "2.20", "3.5", "10.1", "200"));
893+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "2", "2"}).GetVec(), ElementsAre("3.5", "10.1"));
894+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "1", "1"}), "2.20");
895+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "4", "2"}), "200");
896+
ASSERT_THAT(Run({"sort_ro", "list-1", "LIMIT", "5", "2"}), ArrLen(0));
897+
// limits desc
898+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC", "LIMIT", "0", "5"}).GetVec(),
899+
ElementsAre("200", "10.1", "3.5", "2.20", "1.2"));
900+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC", "LIMIT", "2", "2"}).GetVec(),
901+
ElementsAre("3.5", "2.20"));
902+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC", "LIMIT", "1", "1"}), "10.1");
903+
ASSERT_THAT(Run({"sort_ro", "list-1", "DESC", "LIMIT", "5", "2"}), ArrLen(0));
904+
905+
// Test set sort
906+
Run({"del", "set-1"});
907+
Run({"sadd", "set-1", "5.3", "4.4", "60", "99.9", "100", "9"});
908+
ASSERT_THAT(Run({"sort_ro", "set-1"}).GetVec(),
909+
ElementsAre("4.4", "5.3", "9", "60", "99.9", "100"));
910+
ASSERT_THAT(Run({"sort_ro", "set-1", "ALPHA"}).GetVec(),
911+
ElementsAre("100", "4.4", "5.3", "60", "9", "99.9"));
912+
ASSERT_THAT(Run({"sort_ro", "set-1", "DESC"}).GetVec(),
913+
ElementsAre("100", "99.9", "60", "9", "5.3", "4.4"));
914+
ASSERT_THAT(Run({"sort_ro", "set-1", "DESC", "ALPHA"}).GetVec(),
915+
ElementsAre("99.9", "9", "60", "5.3", "4.4", "100"));
916+
917+
// Test intset sort
918+
Run({"del", "intset-1"});
919+
Run({"sadd", "intset-1", "5", "4", "3", "2", "1"});
920+
ASSERT_THAT(Run({"sort_ro", "intset-1"}).GetVec(), ElementsAre("1", "2", "3", "4", "5"));
921+
922+
// Test sorted set sort
923+
Run({"del", "zset-1"});
924+
Run({"zadd", "zset-1", "0", "3.3", "0", "30.1", "0", "8.2"});
925+
ASSERT_THAT(Run({"sort_ro", "zset-1"}).GetVec(), ElementsAre("3.3", "8.2", "30.1"));
926+
ASSERT_THAT(Run({"sort_ro", "zset-1", "ALPHA"}).GetVec(), ElementsAre("3.3", "30.1", "8.2"));
927+
ASSERT_THAT(Run({"sort_ro", "zset-1", "DESC"}).GetVec(), ElementsAre("30.1", "8.2", "3.3"));
928+
ASSERT_THAT(Run({"sort_ro", "zset-1", "DESC", "ALPHA"}).GetVec(),
929+
ElementsAre("8.2", "30.1", "3.3"));
930+
931+
// Test sort with non existent key
932+
Run({"del", "list-2"});
933+
ASSERT_THAT(Run({"sort_ro", "list-2"}), ArrLen(0));
934+
935+
// Test not convertible to double
936+
Run({"lpush", "list-2", "NOTADOUBLE"});
937+
ASSERT_THAT(Run({"sort_ro", "list-2"}),
938+
ErrArg("One or more scores can't be converted into double"));
939+
940+
Run({"set", "foo", "bar"});
941+
ASSERT_THAT(Run({"sort_ro", "foo"}), ErrArg("WRONGTYPE "));
942+
943+
Run({"rpush", "list-3", ""});
944+
ASSERT_THAT(Run({"sort_ro", "list-3"}), "");
945+
946+
Run({"rpush", "list-3", "2", "0", "", "-0.14", "0.12", "-0", "-123123", "7654"});
947+
ASSERT_THAT(Run({"sort_ro", "list-3"}).GetVec(),
948+
ElementsAre("-123123", "-0.14", "", "", "-0", "0", "0.12", "2", "7654"));
949+
950+
Run({"rpush", "NANvalue", "nan"});
951+
ASSERT_THAT(Run({"sort_ro", "NANvalue"}),
952+
ErrArg("One or more scores can't be converted into double"));
953+
954+
// Test store option should not work
955+
ASSERT_THAT(Run({"sort_ro", "list-1", "store", "list-2"}), ErrArg("syntax error"));
956+
}
957+
958+
TEST_F(GenericFamilyTest, SortROBug3636) {
959+
Run({"RPUSH", "foo", "1.100000023841858", "1.100000023841858", "1.100000023841858", "-15710",
960+
"1.100000023841858", "1.100000023841858", "1.100000023841858", "-15710", "-15710",
961+
"1.100000023841858", "-15710", "-15710", "-15710", "-15710", "1.100000023841858", "-15710",
962+
"-15710"});
963+
auto resp = Run({"SORT_RO", "foo", "desc", "alpha"});
964+
ASSERT_THAT(resp, ArrLen(17));
965+
}
966+
872967
TEST_F(GenericFamilyTest, TimeNoKeys) {
873968
auto resp = Run({"time"});
874969
EXPECT_THAT(resp, ArrLen(2));

0 commit comments

Comments
 (0)