You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "uds5501 (via GitHub)" <gi...@apache.org> on 2023/06/17 05:43:51 UTC
[GitHub] [incubator-kvrocks] uds5501 opened a new pull request, #1502: Feat: Implement zunion
uds5501 opened a new pull request, #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502
Fixes : #1461
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236618952
##########
src/commands/cmd_zset.cc:
##########
@@ -1148,6 +1148,78 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ numkeys_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ for (size_t i = 0; i < numkeys_; ++i) {
+ keys_weights_.emplace_back(KeyWeight{GET_OR_RET(parser.TakeStr()), 1});
+ }
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("aggregate")) {
+ std::string aggregate_value = GET_OR_RET(parser.TakeStr());
+ if (util::ToLower(aggregate_value) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(aggregate_value) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(aggregate_value) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ } else if (parser.EatEqICase("weights")) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = parser.TakeFloat();
+ if (!weight) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ }
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
+ std::vector<MemberScore> member_scores;
+ auto s = zset_db.Union(keys_weights_, aggregate_method_, nullptr, &member_scores);
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ std::sort(member_scores.begin(), member_scores.end(), compare_score);
+ output->append(redis::MultiLen(member_scores.size() * (with_scores_ ? 2 : 1)));
+ for (const auto &ms : member_scores) {
+ output->append(redis::BulkString(ms.member));
+ if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
+ }
+ return Status::OK();
+ }
+
+ static CommandKeyRange Range(const std::vector<std::string> &args) {
+ int num_key = *ParseInt<int>(args[1], 10);
+ return {2, 1 + num_key, 1};
Review Comment:
Ah yes, missed that, will update this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234364038
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
@uds5501 If I understand correctly, the initial order of how members are stored is already lexicographically sorted and retrieved in that order. @git-hulk Fix my thought if it is incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1235395905
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
Understood, so I shold lexicographically sort the final result as well , right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1233867663
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -956,6 +956,50 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
require.Equal(t, []redis.Z{{1, "a"}, {2, "b"}, {3, "c"}, {3, "d"}}, rdb.ZRangeWithScores(ctx, "zsetc", 0, -1).Val())
})
+ t.Run(fmt.Sprintf("ZUNION basics - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "zseta", "zsetb", "zsetc")
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 1, Member: "a"})
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 2, Member: "b"})
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 3, Member: "c"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 1, Member: "b"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 2, Member: "c"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 3, Member: "d"})
+
+ zsetInt := []redis.Z{
+ {1, "a"},
+ {3, "b"},
+ {3, "d"},
+ {5, "c"}}
+ require.Equal(t, zsetInt, rdb.ZUnionWithScores(ctx, redis.ZStore{Keys: []string{"zseta", "zsetb"}}).Val())
+ })
+
+ t.Run(fmt.Sprintf("ZUNION with weights - %s", encoding), func(t *testing.T) {
+ zsetInt := []redis.Z{
Review Comment:
I wouldn't recommend relying on the order of test execution. Instead, delete the keys you need at the beginning of the test, populate those keys with values and then test required assertions. In this particular case, if you need sorted sets defined in another test, you can extract dedicated functions that will prepare the sorted sets for you.
On top of that, you can check the `createZset` functions in this file and use it at the beginning of each (if needed) test. This function does exactly what I described.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236237718
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,20 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ if (members && !dst_zset.empty()) {
+ members->reserve(dst_zset.size());
for (const auto &iter : dst_zset) {
- mscores.emplace_back(MemberScore{iter.first, iter.second});
+ members->emplace_back(MemberScore{iter.first, iter.second});
}
- if (saved_cnt) *saved_cnt = mscores.size();
- Overwrite(dst, mscores);
+ if (saved_cnt) *saved_cnt = members->size();
+ std::sort(members->begin(), members->end(), compare_score);
Review Comment:
Makes sense @git-hulk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234278663
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -956,6 +956,50 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
require.Equal(t, []redis.Z{{1, "a"}, {2, "b"}, {3, "c"}, {3, "d"}}, rdb.ZRangeWithScores(ctx, "zsetc", 0, -1).Val())
})
+ t.Run(fmt.Sprintf("ZUNION basics - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "zseta", "zsetb", "zsetc")
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 1, Member: "a"})
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 2, Member: "b"})
+ rdb.ZAdd(ctx, "zseta", redis.Z{Score: 3, Member: "c"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 1, Member: "b"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 2, Member: "c"})
+ rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 3, Member: "d"})
+
+ zsetInt := []redis.Z{
+ {1, "a"},
+ {3, "b"},
+ {3, "d"},
+ {5, "c"}}
+ require.Equal(t, zsetInt, rdb.ZUnionWithScores(ctx, redis.ZStore{Keys: []string{"zseta", "zsetb"}}).Val())
+ })
+
+ t.Run(fmt.Sprintf("ZUNION with weights - %s", encoding), func(t *testing.T) {
+ zsetInt := []redis.Z{
Review Comment:
updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236338013
##########
src/commands/cmd_zset.cc:
##########
@@ -1285,6 +1345,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandZAdd>("zadd", -4, "write", 1, 1, 1),
MakeCmdAttr<CommandZScore>("zscore", 3, "read-only", 1, 1, 1),
MakeCmdAttr<CommandZMScore>("zmscore", -3, "read-only", 1, 1, 1),
MakeCmdAttr<CommandZScan>("zscan", -3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandZUnionStore>("zunionstore", -4, "write", 1, 1, 1), )
+ MakeCmdAttr<CommandZUnionStore>("zunionstore", -4, "write", 1, 1, 1),
+ MakeCmdAttr<CommandZUnion>("zunion", -4, "read-only", 1, 1, 1), )
Review Comment:
Updated both ZUNION and ZUNIONSTORE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236173943
##########
src/commands/cmd_zset.cc:
##########
@@ -1285,6 +1345,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandZAdd>("zadd", -4, "write", 1, 1, 1),
MakeCmdAttr<CommandZScore>("zscore", 3, "read-only", 1, 1, 1),
MakeCmdAttr<CommandZMScore>("zmscore", -3, "read-only", 1, 1, 1),
MakeCmdAttr<CommandZScan>("zscan", -3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandZUnionStore>("zunionstore", -4, "write", 1, 1, 1), )
+ MakeCmdAttr<CommandZUnionStore>("zunionstore", -4, "write", 1, 1, 1),
+ MakeCmdAttr<CommandZUnion>("zunion", -4, "read-only", 1, 1, 1), )
Review Comment:
Looks like the ZUNIONSTORE command wrongly passed the key position. For the ZUNIONSTORE/ZUNION command, its key positions need to calculate by the [numkeys] argument. Can refer to the `CommandZMPop::Range` implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236223511
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,20 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ if (members && !dst_zset.empty()) {
+ members->reserve(dst_zset.size());
for (const auto &iter : dst_zset) {
- mscores.emplace_back(MemberScore{iter.first, iter.second});
+ members->emplace_back(MemberScore{iter.first, iter.second});
}
- if (saved_cnt) *saved_cnt = mscores.size();
- Overwrite(dst, mscores);
+ if (saved_cnt) *saved_cnt = members->size();
+ std::sort(members->begin(), members->end(), compare_score);
Review Comment:
Underlying storage will sort them for ZUNIONSTORE, but we don’t use the storage for ZUNION, so i think to keep the response consistent, we’ll need to sort it at this level (atleast for ZUNION) command
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1502: Add support of the new command ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234756365
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
I think it'd be better to use the common parser like other commands to make the codebase more coherent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1235451744
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
Yes, exactly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234291141
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
@torwig / @git-hulk needed an opinion on this. If the scores of both the members are same during sorting, do we want to also lexicographically check the members as tie breakers or are we okay with them being in the order which they were originally present in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236170266
##########
tests/cppunit/command_parser_test.cc:
##########
@@ -26,9 +26,10 @@ TEST(CommandParser, Parse) {
// [ HELLO i1 v1 | HI v2 ] [X i2 | Y]
std::vector<std::string> c1{"hello", "1", "a"}, c2{"hi", "b"}, c3{"hi", "c", "x", "2"}, c4{"hello", "3", "d", "y"},
c5{"hi", "e", "y"}, c6{"y"}, d1{"hello"}, d2{"hi"}, d3{"hello", "no-int"}, d4{"x", "1", "y"},
- d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"};
+ d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"}, d7{"float_test", "1.1", "c"};
Review Comment:
```suggestion
d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"}, d7{"float_test", "1.1"};
```
Adding extra "c" will cause invalid syntax in the parser since it cannot be recognized.
##########
tests/cppunit/command_parser_test.cc:
##########
@@ -26,9 +26,10 @@ TEST(CommandParser, Parse) {
// [ HELLO i1 v1 | HI v2 ] [X i2 | Y]
std::vector<std::string> c1{"hello", "1", "a"}, c2{"hi", "b"}, c3{"hi", "c", "x", "2"}, c4{"hello", "3", "d", "y"},
c5{"hi", "e", "y"}, c6{"y"}, d1{"hello"}, d2{"hi"}, d3{"hello", "no-int"}, d4{"x", "1", "y"},
- d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"};
+ d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"}, d7{"float_test", "1.1", "c"};
Review Comment:
```suggestion
d5{"hello", "1", "v", "hi", "v"}, d6{"hello", "1"}, d7{"float_test", "1.1"};
```
Adding an extra "c" will cause invalid syntax in the parser since it cannot be recognized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236254483
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
> Btw for parsing, imo, a simple option_iterator++ would also work, wdyt?
It is firstly errorphone, e.g. you just do not realize your dead loop here.
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
> Btw for parsing, imo, a simple option_iterator++ would also work, wdyt?
It is firstly errorphone, e.g. you just do not realize your dead loop here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234754184
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
> @uds5501 If I understand correctly, the initial order of how members are stored is already lexicographically sorted and retrieved in that order. @git-hulk Fix my thought if it is incorrect.
Yes, that's right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1235516065
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,15 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) { return score1.score < score2.score; };
Review Comment:
Incorporated this sorting logic, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236637074
##########
src/commands/cmd_zset.cc:
##########
@@ -1148,6 +1148,78 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ numkeys_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ for (size_t i = 0; i < numkeys_; ++i) {
+ keys_weights_.emplace_back(KeyWeight{GET_OR_RET(parser.TakeStr()), 1});
+ }
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("aggregate")) {
+ std::string aggregate_value = GET_OR_RET(parser.TakeStr());
+ if (util::ToLower(aggregate_value) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(aggregate_value) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(aggregate_value) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ } else if (parser.EatEqICase("weights")) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = parser.TakeFloat();
+ if (!weight) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ }
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
+ std::vector<MemberScore> member_scores;
+ auto s = zset_db.Union(keys_weights_, aggregate_method_, nullptr, &member_scores);
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ std::sort(member_scores.begin(), member_scores.end(), compare_score);
+ output->append(redis::MultiLen(member_scores.size() * (with_scores_ ? 2 : 1)));
+ for (const auto &ms : member_scores) {
+ output->append(redis::BulkString(ms.member));
+ if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
+ }
+ return Status::OK();
+ }
+
+ static CommandKeyRange Range(const std::vector<std::string> &args) {
+ int num_key = *ParseInt<int>(args[1], 10);
+ return {2, 1 + num_key, 1};
Review Comment:
updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236228585
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,20 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ if (members && !dst_zset.empty()) {
+ members->reserve(dst_zset.size());
for (const auto &iter : dst_zset) {
- mscores.emplace_back(MemberScore{iter.first, iter.second});
+ members->emplace_back(MemberScore{iter.first, iter.second});
}
- if (saved_cnt) *saved_cnt = mscores.size();
- Overwrite(dst, mscores);
+ if (saved_cnt) *saved_cnt = members->size();
+ std::sort(members->begin(), members->end(), compare_score);
Review Comment:
@uds5501 OK, then it's good for me. How about moving the sort logic to the ZUNION command since we don't need to sort for the union store?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236169700
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,20 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ if (members && !dst_zset.empty()) {
+ members->reserve(dst_zset.size());
for (const auto &iter : dst_zset) {
- mscores.emplace_back(MemberScore{iter.first, iter.second});
+ members->emplace_back(MemberScore{iter.first, iter.second});
}
- if (saved_cnt) *saved_cnt = mscores.size();
- Overwrite(dst, mscores);
+ if (saved_cnt) *saved_cnt = members->size();
+ std::sort(members->begin(), members->end(), compare_score);
Review Comment:
Sorry, maybe I misunderstand your previous comment. We don't need to sort members here, because the underlying storage will sort them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234245405
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
Makes sense.
Btw for parsing, imo, a simple `option_iterator++` would also work, wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1234246941
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
+ } else {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
+ std::vector<MemberScore> member_scores;
+ uint64_t size = 0;
+ auto s = zset_db.Union(keys_weights_, aggregate_method_, &size, &member_scores);
Review Comment:
ack, incorporating this suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1233778002
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
It would go into the dead loop if passed the WITHSCORES flag here. And for the flag parse should take a look at the implementation like: https://github.com/apache/incubator-kvrocks/blob/91d9abb6c1169db7b91409456b8bf4bd478309dc/src/commands/cmd_zset.cc#L287
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1502: Feat: Implement zunion
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1233856234
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
+ } else {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
+ std::vector<MemberScore> member_scores;
+ uint64_t size = 0;
+ auto s = zset_db.Union(keys_weights_, aggregate_method_, &size, &member_scores);
Review Comment:
Since you don't use the `size` variable you could remove it and pass a `nullptr` to the `Union` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236350539
##########
src/types/redis_zset.cc:
##########
@@ -744,16 +754,20 @@ rocksdb::Status ZSet::UnionStore(const Slice &dst, const std::vector<KeyWeight>
}
}
}
- if (!dst_zset.empty()) {
- std::vector<MemberScore> mscores;
- mscores.reserve(dst_zset.size());
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ if (members && !dst_zset.empty()) {
+ members->reserve(dst_zset.size());
for (const auto &iter : dst_zset) {
- mscores.emplace_back(MemberScore{iter.first, iter.second});
+ members->emplace_back(MemberScore{iter.first, iter.second});
}
- if (saved_cnt) *saved_cnt = mscores.size();
- Overwrite(dst, mscores);
+ if (saved_cnt) *saved_cnt = members->size();
+ std::sort(members->begin(), members->end(), compare_score);
Review Comment:
Incorporated this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1502: Add support of the new command ZUNION
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1236467536
##########
src/commands/cmd_zset.cc:
##########
@@ -1148,6 +1148,78 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ numkeys_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ for (size_t i = 0; i < numkeys_; ++i) {
+ keys_weights_.emplace_back(KeyWeight{GET_OR_RET(parser.TakeStr()), 1});
+ }
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("aggregate")) {
+ std::string aggregate_value = GET_OR_RET(parser.TakeStr());
+ if (util::ToLower(aggregate_value) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(aggregate_value) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(aggregate_value) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ } else if (parser.EatEqICase("weights")) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = parser.TakeFloat();
+ if (!weight) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ } else if (parser.EatEqICase("withscores")) {
+ with_scores_ = true;
+ }
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ redis::ZSet zset_db(svr->storage, conn->GetNamespace());
+ std::vector<MemberScore> member_scores;
+ auto s = zset_db.Union(keys_weights_, aggregate_method_, nullptr, &member_scores);
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ auto compare_score = [](const MemberScore &score1, const MemberScore &score2) {
+ if (score1.score == score2.score) {
+ return score1.member < score2.member;
+ }
+ return score1.score < score2.score;
+ };
+ std::sort(member_scores.begin(), member_scores.end(), compare_score);
+ output->append(redis::MultiLen(member_scores.size() * (with_scores_ ? 2 : 1)));
+ for (const auto &ms : member_scores) {
+ output->append(redis::BulkString(ms.member));
+ if (with_scores_) output->append(redis::BulkString(util::Float2String(ms.score)));
+ }
+ return Status::OK();
+ }
+
+ static CommandKeyRange Range(const std::vector<std::string> &args) {
+ int num_key = *ParseInt<int>(args[1], 10);
+ return {2, 1 + num_key, 1};
Review Comment:
I'm not sure, but since `ZUnion` and `ZUnionStore` differs that `ZunionStore` has the `destination` argument, should their `Range` functions return different values (at least in one position)? @git-hulk @PragmaTwice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] uds5501 commented on a diff in pull request #1502: Feat: Implement ZUNION
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1502:
URL: https://github.com/apache/incubator-kvrocks/pull/1502#discussion_r1235515491
##########
src/commands/cmd_zset.cc:
##########
@@ -819,6 +819,81 @@ class CommandZMScore : public Commander {
}
};
+class CommandZUnion : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ auto parse_result = ParseInt<int>(args[1], 10);
+ if (!parse_result) {
+ return {Status::RedisParseErr, errValueNotInteger};
+ }
+
+ numkeys_ = *parse_result;
+ if (numkeys_ > args.size() - 2) {
+ return {Status::RedisParseErr, errInvalidSyntax};
+ }
+
+ size_t key_iterator = 0;
+ while (key_iterator < numkeys_) {
+ keys_weights_.emplace_back(KeyWeight{args[key_iterator + 2], 1});
+ key_iterator++;
+ }
+
+ size_t option_iterator = 2 + numkeys_;
+ while (option_iterator < args.size()) {
+ if (util::ToLower(args[option_iterator]) == "aggregate" && option_iterator + 1 < args.size()) {
+ if (util::ToLower(args[option_iterator + 1]) == "sum") {
+ aggregate_method_ = kAggregateSum;
+ } else if (util::ToLower(args[option_iterator + 1]) == "min") {
+ aggregate_method_ = kAggregateMin;
+ } else if (util::ToLower(args[option_iterator + 1]) == "max") {
+ aggregate_method_ = kAggregateMax;
+ } else {
+ return {Status::RedisParseErr, "aggregate param error"};
+ }
+ option_iterator += 2;
+ } else if (util::ToLower(args[option_iterator]) == "weights" && option_iterator + numkeys_ < args.size()) {
+ size_t k = 0;
+ while (k < numkeys_) {
+ auto weight = ParseFloat(args[option_iterator + k + 1]);
+ if (!weight || std::isnan(*weight)) {
+ return {Status::RedisParseErr, "weight is not a double or out of range"};
+ }
+ keys_weights_[k].weight = *weight;
+ k++;
+ }
+ option_iterator += numkeys_ + 1;
+ } else if (util::ToLower(args[option_iterator]) == "withscores") {
+ with_scores_ = true;
Review Comment:
Incorporated this change, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org