You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/10/21 09:59:40 UTC
[GitHub] [incubator-kvrocks] manchurio opened a new pull request, #1022: Implement the ZADD options
manchurio opened a new pull request, #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022
This closes #1018
Has passed the unit test with go.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1001911867
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
Review Comment:
Not only determine whether the key exists, but also get the associated value. And in your code, it finds value twice, bad for performance.
--
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] tanruixiang commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287193692
> And in my opinion, I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.
Got it, thanks a lot for your suggestion. It should be that I misunderstood this "request for changes". (Because if someone else suggests a little modification, I prefer to click on github to complete the modificationš¤£)
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002891849
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
Yes, you're right, my bad for misunderstanding the break logic.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006680534
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
> @torwig Could you explain why? In my opinion, I prefer to use "using" instead of 'typedef', but for consistency with other code. I choose this way.
Just use `class A{};` instead of `typedef class A{} A;`. And you do not need to care consistency on `typedef`, we will replace them by `using`.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287895141
@manchurio By the way, where are the unit tests? :)
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002710721
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
@git-hulk I think that `options` is OK since the `Redis` documentation names them as `ZADD options`. Perhaps the function name should be more specific like `parseAddOptions`. However, this method belongs to the class `CommandZAdd` so it may be clear that they are ZAdd-options. What do you think?
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002873194
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
It should not returns here.
eg.
del z1
zadd z1 1 a
zadd z1 nx 2 b 2 a // b should be added since it is new.
If it returns here, b has no chance to add.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002955636
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
Ok, changed to parseFlags now.
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286954886
> Oh, I notice the cppcheck false alarm. I think you can change `--std=c++11` to `17` here: https://github.com/apache/incubator-kvrocks/blob/unstable/x.py#L162
>
> @manchurio
```
src/commands/redis_cmd.cc:2432:19: warning: Suspicious condition. The result of find() is an iterator, but it is not properly checked. [stlIfFind]
if (auto it = options.find(option); it != options.end()) {
^
```
It seems cppcheck wrongly reports @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] PragmaTwice commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287996635
The file permission of `x.py` has changed from 755 to 644, which is weird. Could you revert it?
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002712118
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
Yes, flags or options are OK. I saw that it calls 'options' when parsing but used `validateFlags` when validating, so I think it will be better to keep consistent if possible.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002873194
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
It should not returns here.
eg.
del z1
zadd z1 1 a
zadd z1 nx 2 b 2 a // b should be added since it is new.
If it returns here, b has no chance to be added.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006701625
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
OK, I also prefer class.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002873194
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
It should not return here.
eg.
del z1
zadd z1 1 a
zadd z1 nx 2 b 2 a // b should be added since it is new.
If it returns here, b has no chance to add.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006680534
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
> @torwig Could you explain why? In my opinion, I prefer to use "using" instead of 'typedef', but for consistency with other code. I choose this way.
Just use `class A{};` instead of `typedef class A{} A;`. And you do not need care consistent on `typedef`, we will replace them by `using`.
--
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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002317588
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
Review Comment:
Thank you very much for your contribution. There may be duplicate values here, so the upper limit is not necessarily 6. For example, the following is the running result of redis.
```
127.0.0.1:6379> ZADD myzset 3 "test"
(integer) 1
127.0.0.1:6379> ZADD myzset XX XX XX XX XX XX XX INCR 3 "test"
"6"
```
--
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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1001873448
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
Review Comment:
```suggestion
if (options.count(option)) {
flags_ |= options[option];
index++;
} else {
break;
}
```
--
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] tanruixiang commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287199649
> Actually I like the original code more, since the iterator is used in the true-branch block to avoid a double search (the upper bound is actually not O(1) due to collision, and even O(1) DOES NOT imples no cost).
>
> And I think this change does not improve readability, as the use of iterators is natural and easy to understand in c++. The false positives of cppcheck are entirely due to his own problems, and we should not modify the code logic just for that.
Maybe it's my bad habit, I always try not to use iterators in code snippets that are similar to algorithm problems. I will try to use iterators more in the future.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002564533
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
+ if (flags & kZSetNX) {
Review Comment:
I think it's not a correct condition. Because if there is a non-empty sorted set and you try to add a new member WITH the option `NX`, `kvrocks` doesn't add this element but `Redis` does. The root cause is that if the flag `NX` is set => then for loop does nothing because every time `continue` is executed (of course, if there is at least one element in the sorted set).
For example:
`Kvrocks`
```
[yaroslav@localhost redis]$ ./src/redis-cli -p 6666
127.0.0.1:6666> zadd ss 1 a 2 b 3 c
(integer) 3
127.0.0.1:6666> zrange ss 0 100
1) "a"
2) "b"
3) "c"
127.0.0.1:6666> zadd ss NX 4 d
(integer) 0 (<-- NOT added but this element is NEW)
127.0.0.1:6666> zrange ss 0 100
1) "a"
2) "b"
3) "c"
```
Compared to `Redis`:
```
[yaroslav@localhost redis]$ ./src/redis-cli
127.0.0.1:6379> zadd ss 1 a 2 b 3 c
(integer) 3
127.0.0.1:6379> zrange ss 0 100
1) "a"
2) "b"
3) "c"
127.0.0.1:6379> zadd ss NX 4 d
(integer) 1 (<-- ADDED)
127.0.0.1:6379> zrange ss 0 100
1) "a"
2) "b"
3) "c"
4) "d"
```
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,19 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
};
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ for (unsigned i = 2; i < args.size(); i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
+ }
+}
+
+Status CommandZAdd::validateFlags() {
+ if (!flags_) {
+ return {};
Review Comment:
I'd prefer to write `Status::OK()` explicitly. At least it's used over the codebase (consistency).
The same is at the end of this function.
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,19 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
};
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ for (unsigned i = 2; i < args.size(); i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
+ }
+}
+
+Status CommandZAdd::validateFlags() {
Review Comment:
This function can be marked as `const`.
##########
src/commands/redis_cmd.cc:
##########
@@ -2365,12 +2366,19 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
+ if (auto s = validateFlags(); !s.IsOK()) {
+ return s;
+ }
+ if (auto left = (args.size() - index); left > 0) {
+ if ((flags_ & kZSetIncr) && left != 2) {
+ return Status(Status::RedisParseErr, "INCR option supports a single increment-element pair");
+ } else if (left % 2 != 0)
Review Comment:
To be consistent you should add curly braces to the statement inside `else if` clause.
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,19 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Review Comment:
This line can be safely moved before the actual use of `incr` - before `if`-statement.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286986765
@manchurio Need to use gofmt to format the test case
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002867599
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -93,6 +93,82 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
require.Equal(t, float64(30), rdb.ZScore(ctx, "ztmp", "x").Val())
})
+ t.Run(fmt.Sprintf("ZSET ZADD INCR option supports a single pair - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Contains(t, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}, {Member: "adc"}}}).Err(),
+ "INCR option supports a single increment-element pair")
+ })
+
+ t.Run(fmt.Sprintf("ZSET ZADD IncrMixedOtherOptions - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, "1.5", rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Val())
+ require.Equal(t, redis.Nil, rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Err())
+ require.Equal(t, "3", rdb.Do(ctx, "zadd", "ztmp", "xx", "xx", "xx", "xx", "incr", "1.5", "abc").Val())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{XX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 3.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Err())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+ })
+
+ t.Run(fmt.Sprintf("ZSET ZADD LT/GT with other options - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
Review Comment:
Great, it's more convenient.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002873194
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
It should not returns here.
eg.
del z1
zadd z1 1 a
zadd z1 nx 2 b 2 a // b should be added since it is new.
If it returns here, b has no chance to be add.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002956330
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
+ }
double old_score = DecodeDouble(old_score_bytes.data());
- if (flags == kZSetIncr) {
+ if (flags & kZSetIncr) {
+ if (lt && (*mscores)[i].score >= 0) {
+ continue;
+ } else if (gt && (*mscores)[i].score <= 0) {
+ continue;
+ }
(*mscores)[i].score += old_score;
if (std::isnan((*mscores)[i].score)) {
return rocksdb::Status::InvalidArgument("resulting score is not a number (NaN)");
}
}
if ((*mscores)[i].score != old_score) {
+ if (lt && (*mscores)[i].score >= old_score) {
Review Comment:
merged
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006916341
##########
src/commands/redis_cmd.cc:
##########
@@ -2377,12 +2378,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseFlags(args, index);
+ if (auto s = validateFlags(); !s.IsOK()) {
+ return s;
+ }
+ if (auto left = (args.size() - index); left > 0) {
Review Comment:
How about left = 0? if requests have only flags but no member and score?
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1007159984
##########
src/commands/redis_cmd.cc:
##########
@@ -2377,12 +2378,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseFlags(args, index);
+ if (auto s = validateFlags(); !s.IsOK()) {
+ return s;
+ }
+ if (auto left = (args.size() - index); left > 0) {
Review Comment:
It's a bug. It's fixed now.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006575765
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
@torwig Could you explain why?
In my opinion, I prefer to use "using" instead of 'typedef', but for consistency with other code. I choose this way.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006578871
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
@manchurio Because it's just a C++ type definition:
```
struct Name {
// members
// methods
};
class OtherName {
// members
// methods
};
```
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1294500429
Sorry, touched wrong button. :rofl:
--
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] tanruixiang commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288026895
> @manchurio @tanruixiang Yes, I meant the unit tests that are written in C++. Related to the sorted set unit tests are located in `tests/cppunit/t_zset_test.cc` and test only one component - `class ZSet`. On the other hand, tests in Go/TCL test the entire `kvrocks` (storage + protocol + network).
By the way, after everyone's efforts, there are only Go tests now.š
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288026115
@manchurio @tanruixiang Yes, I meant the unit tests that are written in C++. Related to the sorted set unit tests are located in `tests/cppunit/t_zset_test.cc` and test only one component - `class ZSet`.
On the other hand, tests in Go/TCL test the entire `kvrocks` (storage + protocol + network).
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002955801
##########
src/commands/redis_cmd.cc:
##########
@@ -2400,19 +2409,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ bool incr = (flags_ & kZSetIncr) != 0;
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags() const;
};
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ for (unsigned i = 2; i < args.size(); i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
+ }
+}
+
+Status CommandZAdd::validateFlags() const {
+ if (!flags_) {
+ return Status::OK();
+ }
+ bool nx = (flags_ & kZSetNX) != 0;
+ bool xx = (flags_ & kZSetXX) != 0;
+ bool lt = (flags_ & kZSetLT) != 0;
+ bool gt = (flags_ & kZSetGT) != 0;
+ if (nx && xx) {
+ return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible");
+ }
+ if (lt && gt) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
+ if (lt && nx) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
+ if (gt && nx) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
Review Comment:
merged.
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286941221
> You can just run `./x.py format` to format the source code rather than manual formatting.
got it,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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002317588
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
Review Comment:
Thank you very much for your contribution. There may be duplicate flags here, so the upper limit is not necessarily 6. For example, the following is the running result of redis.
```
127.0.0.1:6379> ZADD myzset 3 "test"
(integer) 1
127.0.0.1:6379> ZADD myzset XX XX XX XX XX XX XX INCR 3 "test"
"6"
```
--
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] tisonkun merged pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002712426
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
@git-hulk Now I see. Yes, you are right here. There should be one name everywhere for them (flags or options).
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002955286
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
+ }
double old_score = DecodeDouble(old_score_bytes.data());
- if (flags == kZSetIncr) {
+ if (flags & kZSetIncr) {
+ if (lt && (*mscores)[i].score >= 0) {
+ continue;
+ } else if (gt && (*mscores)[i].score <= 0) {
+ continue;
+ }
Review Comment:
merged
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006702244
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
+ explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {}
+
+ bool HasNX() const { return (flags & kZSetNX) != 0; }
+ bool HasXX() const { return (flags & kZSetXX) != 0; }
+ bool HasLT() const { return (flags & kZSetLT) != 0; }
+ bool HasGT() const { return (flags & kZSetGT) != 0; }
+ bool HasCH() const { return (flags & kZSetCH) != 0; }
+ bool HasIncr() const { return (flags & kZSetIncr) != 0; }
+ bool HasFlag() const { return flags != 0; }
+
+ void SetFlag(ZSetFlags setFlags) { flags |= setFlags; }
+
+ static const ZAddFlags Incr() { return ZAddFlags{kZSetIncr}; }
+
+ static const ZAddFlags Default() { return ZAddFlags{0}; }
+
+ private:
+ uint8_t flags = 0;
+} ZAddFlags;
Review Comment:
Use class instead of typedef struct
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006575765
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
@torwig Could you explain why?
In my opinion, I prefer use "using" rather than "typedef", but for consistency with other code. I choose this way.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287180370
> If you only need to determine whether the key exists, you can use count directly, the iterator is redundant. Also doing this will not trigger an error from cppcheck. (By the way, C++20 has [contains](https://en.cppreference.com/w/cpp/container/unordered_map/contains) to do this thing. )
Hi @tanruixiang,
Actually I like the original code more, since the iterator is used in the true-branch block to avoid a double search (the upper bound is actually not O(1) due to collision, and even O(1) DOES NOT imples no cost). And I think this change does not improve readability, as the use of iterators is natural and easy to understand in c++. The false positives of cppcheck are entirely due to his own problems, and we should not modify the code logic just for that.
And I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1287230942
> > And in my opinion, I prefer just a comment rather than "request for changes" for just a trivial and purely code-smell review message, in order to be nice to new contributors.
>
> Got it, thanks a lot for your suggestion. It should be that I misunderstood this "request for changes". I originally thought that only use "request for changes" can be clicked on github to modify. (Because if someone else suggests a little modification, I prefer to click on github to complete the modificationš¤£)
In the github review page, you can select "comment", "request for changes" or "approve". I think the first option is relatively "milder".
--
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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1003148347
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,31 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = flags.HasLT();
+ bool gt = flags.HasGT();
Review Comment:
ditto.
##########
src/commands/redis_cmd.cc:
##########
@@ -2400,19 +2409,65 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ bool incr = flags_.HasIncr();
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT();
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
+ ZAddFlags flags_{0};
+
+ void parseFlags(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags() const;
};
+void CommandZAdd::parseFlags(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ for (unsigned i = 2; i < args.size(); i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_.SetFlag(it->second);
+ index++;
+ } else {
+ break;
+ }
+ }
+}
+
+Status CommandZAdd::validateFlags() const {
+ if (!flags_.HasFlag()) {
+ return Status::OK();
+ }
+ bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT();
+ if (nx && xx) {
+ return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible");
+ }
+ if ((lt && gt) || (lt && nx) || (gt && nx)) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
Review Comment:
ditto.
##########
src/commands/redis_cmd.cc:
##########
@@ -2400,19 +2409,65 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ bool incr = flags_.HasIncr();
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = flags_.HasNX(), xx = flags_.HasXX(), lt = flags_.HasLT(), gt = flags_.HasGT();
+ if ((nx || xx || lt || gt) && old_score == new_score &&
Review Comment:
`incr` ,`nx`, `xx`, `lt` and `gt` are rebundant, It can be obtained directly using `Has*`.
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288027983
> The file permission of `x.py` has changed from 755 to 644, which is weird. Could you revert it?
The file permission of `x.py` has changed from 644 to 755 now. @PragmaTwice @tisonkun
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288018572
> Please revert changes on `x.py`.
> The file permission of `x.py` has changed from 755 to 644, which is weird. Could you revert it?
OK. I'll figure out what happened, then fix it later.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002712834
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
š Yes, I didn't claim the context clearly in the first comment.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286850316
Oh, I notice the cppcheck false alarm. I think you can change c++11 to 17 here: https://github.com/apache/incubator-kvrocks/blob/unstable/x.py#L162
@manchurio
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002426833
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
Review Comment:
Thanks for your review.I will fix it later.
--
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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1001944668
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
Review Comment:
Yes. But after all, the average time complexity is O(1), and the `options` are very small, so there is basically no gap in performance. Readability is probably more important here. Of course, your implementation is fine too.
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1001911867
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
Review Comment:
Not only determine whether the key exists, but also get the associated value. And in your code, it will find value twice, bad for performance.
--
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] tanruixiang commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288021552
> > @manchurio By the way, where are the unit tests? :)
>
> The unit tests of zadd options are in zset_test.go
Hi. What he meant maybe the `test/cppunit/t_set_test.cc`.
--
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] tanruixiang commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002317588
##########
src/commands/redis_cmd.cc:
##########
@@ -2385,18 +2393,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
+ bool incr = (flags_ & kZSetIncr) != 0;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
-};
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags();
+};
+
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ constexpr unsigned max_index = 6;
+ for (unsigned i = 2; i < max_index; i++) {
Review Comment:
Thank you very much for your contribution. There may be duplicate values here, so the upper limit is not necessarily 6. For example, the following is the running result of redis.
```
127.0.0.1:6379> ZADD myzset 3 "test"
(integer) 1
127.0.0.1:6379> ZADD myzset XX XX XX XX XX XX XX INCR 3 "test"
"6"
127.0.0.1:6379>
```
--
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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006575765
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
@torwig Could you explain why?
In my opinion, I prefer to use "using" rather than "typedef", but for consistency with other code. I choose this way.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288607880
@manchurio Thank you, I'll have a look at it as soon as possible.
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288601843
> @manchurio You can also consider using something like the following to encapsulate the logic with options and eliminate code duplication:
>
> ```
> struct ZAddOptions {
> ZAddOptions() {}
> explicit ZAddOptions(uint8_t flags) : flags(flags) {}
>
> bool IsXX() const { return (flags & kZSetNX) != 0; }
> // other methods to ask if the specific flag was set
>
> uint8_t flags = 0;
> };
> ```
>
> Of course, you can add methods like `SetXX` to encapsulate set operations as well (to not spread bit operations over the codebase).
This refactoring has been completed
--
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] manchurio commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288016483
> @manchurio By the way, where are the unit tests? :)
Th unit tests of zadd options are all in zset_test.go
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002660563
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
+ }
double old_score = DecodeDouble(old_score_bytes.data());
- if (flags == kZSetIncr) {
+ if (flags & kZSetIncr) {
+ if (lt && (*mscores)[i].score >= 0) {
+ continue;
+ } else if (gt && (*mscores)[i].score <= 0) {
+ continue;
+ }
(*mscores)[i].score += old_score;
if (std::isnan((*mscores)[i].score)) {
return rocksdb::Status::InvalidArgument("resulting score is not a number (NaN)");
}
}
if ((*mscores)[i].score != old_score) {
+ if (lt && (*mscores)[i].score >= old_score) {
Review Comment:
Same with above, can merge those two conditions
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
+ }
double old_score = DecodeDouble(old_score_bytes.data());
- if (flags == kZSetIncr) {
+ if (flags & kZSetIncr) {
+ if (lt && (*mscores)[i].score >= 0) {
+ continue;
+ } else if (gt && (*mscores)[i].score <= 0) {
+ continue;
+ }
Review Comment:
```suggestion
if (lt && (*mscores)[i].score >= 0) {
continue;
} else if (gt && (*mscores)[i].score <= 0) {
continue;
}
```
Can merge into one line with `||`
##########
src/commands/redis_cmd.cc:
##########
@@ -2400,19 +2409,74 @@ class CommandZAdd : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
int ret;
+ double old_score = member_scores_[0].score;
Redis::ZSet zset_db(svr->storage_, conn->GetNamespace());
- rocksdb::Status s = zset_db.Add(args_[1], 0, &member_scores_, &ret);
+ rocksdb::Status s = zset_db.Add(args_[1], flags_, &member_scores_, &ret);
if (!s.ok()) {
return Status(Status::RedisExecErr, s.ToString());
}
- *output = Redis::Integer(ret);
+ bool incr = (flags_ & kZSetIncr) != 0;
+ if (incr) {
+ auto new_score = member_scores_[0].score;
+ bool nx = (flags_ & kZSetNX), xx = (flags_ & kZSetXX), lt = (flags_ & kZSetLT), gt = (flags_ & kZSetGT);
+ if ((nx || xx || lt || gt) && old_score == new_score &&
+ ret == 0) { // not the first time using incr && score not changed
+ *output = Redis::NilString();
+ return Status::OK();
+ }
+ *output = Redis::BulkString(Util::Float2String(new_score));
+ } else {
+ *output = Redis::Integer(ret);
+ }
return Status::OK();
}
private:
std::vector<MemberScore> member_scores_;
+ uint8_t flags_ = 0;
+
+ void parseOptions(const std::vector<std::string> &args, unsigned &index);
+ Status validateFlags() const;
};
+void CommandZAdd::parseOptions(const std::vector<std::string> &args, unsigned &index) {
+ std::unordered_map<std::string, ZSetFlags> options = {{"xx", kZSetXX}, {"nx", kZSetNX}, {"ch", kZSetCH},
+ {"lt", kZSetLT}, {"gt", kZSetGT}, {"incr", kZSetIncr}};
+ for (unsigned i = 2; i < args.size(); i++) {
+ auto option = Util::ToLower(args[i]);
+ auto it = options.find(option);
+ if (it != options.end()) {
+ flags_ |= it->second;
+ index++;
+ } else {
+ break;
+ }
+ }
+}
+
+Status CommandZAdd::validateFlags() const {
+ if (!flags_) {
+ return Status::OK();
+ }
+ bool nx = (flags_ & kZSetNX) != 0;
+ bool xx = (flags_ & kZSetXX) != 0;
+ bool lt = (flags_ & kZSetLT) != 0;
+ bool gt = (flags_ & kZSetGT) != 0;
+ if (nx && xx) {
+ return Status(Status::RedisParseErr, "XX and NX options at the same time are not compatible");
+ }
+ if (lt && gt) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
+ if (lt && nx) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
+ if (gt && nx) {
+ return Status(Status::RedisParseErr, errZSetLTGTNX);
+ }
Review Comment:
```suggestion
if ((lt && gt) || (lt && nx) || (gt && nx)) {
return Status(Status::RedisParseErr, errZSetLTGTNX);
}
```
##########
src/commands/redis_cmd.cc:
##########
@@ -2380,12 +2381,20 @@ class CommandSInterStore : public Commander {
class CommandZAdd : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() % 2 != 0) {
- return Status(Status::RedisParseErr, errInvalidSyntax);
+ unsigned index = 2;
+ parseOptions(args, index);
Review Comment:
Can use consistent name like `parseFlags`
##########
src/types/redis_zset.cc:
##########
@@ -71,19 +72,35 @@ rocksdb::Status ZSet::Add(const Slice &user_key, uint8_t flags, std::vector<Memb
}
added_member_keys.insert(member_key);
+ bool lt = (flags & kZSetLT) != 0;
+ bool gt = (flags & kZSetGT) != 0;
+
if (metadata.size > 0) {
std::string old_score_bytes;
s = db_->Get(rocksdb::ReadOptions(), member_key, &old_score_bytes);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
+ if (!s.IsNotFound() && (flags & kZSetNX)) {
+ continue;
Review Comment:
Can just return instead of continuing 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] tisonkun commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1002719444
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -93,6 +93,82 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
require.Equal(t, float64(30), rdb.ZScore(ctx, "ztmp", "x").Val())
})
+ t.Run(fmt.Sprintf("ZSET ZADD INCR option supports a single pair - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Contains(t, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}, {Member: "adc"}}}).Err(),
+ "INCR option supports a single increment-element pair")
+ })
+
+ t.Run(fmt.Sprintf("ZSET ZADD IncrMixedOtherOptions - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, "1.5", rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Val())
+ require.Equal(t, redis.Nil, rdb.Do(ctx, "zadd", "ztmp", "nx", "nx", "nx", "nx", "incr", "1.5", "abc").Err())
+ require.Equal(t, "3", rdb.Do(ctx, "zadd", "ztmp", "xx", "xx", "xx", "xx", "incr", "1.5", "abc").Val())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{XX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 3.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{GT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Err())
+
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, 1.5, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{NX: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: -1.5}}}).Val())
+ require.Equal(t, 0.0, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
+ require.Equal(t, redis.Nil, rdb.ZAddArgsIncr(ctx, "ztmp", redis.ZAddArgs{LT: true, Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Err())
+ })
+
+ t.Run(fmt.Sprintf("ZSET ZADD LT/GT with other options - %s", encoding), func(t *testing.T) {
+ rdb.Del(ctx, "ztmp")
+ require.Equal(t, int64(1), rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
Review Comment:
```suggestion
require.EqualValues(t, 1, rdb.ZAddArgs(ctx, "ztmp", redis.ZAddArgs{Members: []redis.Z{{Member: "abc", Score: 1.5}}}).Val())
```
nit: better assertions.
ditto others.
--
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 #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1003204983
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
+ explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {}
+
+ bool HasNX() const { return (flags & kZSetNX) != 0; }
+ bool HasXX() const { return (flags & kZSetXX) != 0; }
+ bool HasLT() const { return (flags & kZSetLT) != 0; }
+ bool HasGT() const { return (flags & kZSetGT) != 0; }
+ bool HasCH() const { return (flags & kZSetCH) != 0; }
+ bool HasIncr() const { return (flags & kZSetIncr) != 0; }
+ bool HasFlag() const { return flags != 0; }
+
+ void SetFlag(ZSetFlags setFlags) { flags |= setFlags; }
+
+ static const ZAddFlags Incr() { return ZAddFlags{kZSetIncr}; }
+
+ static const ZAddFlags Default() { return ZAddFlags{0}; }
+
+ private:
+ uint8_t flags = 0;
+} ZAddFlags;
Review Comment:
And leave only `};` here, removing `ZAddFlags`.
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
+ explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {}
+
+ bool HasNX() const { return (flags & kZSetNX) != 0; }
+ bool HasXX() const { return (flags & kZSetXX) != 0; }
+ bool HasLT() const { return (flags & kZSetLT) != 0; }
+ bool HasGT() const { return (flags & kZSetGT) != 0; }
+ bool HasCH() const { return (flags & kZSetCH) != 0; }
+ bool HasIncr() const { return (flags & kZSetIncr) != 0; }
+ bool HasFlag() const { return flags != 0; }
Review Comment:
I'd rename this, for example, to `HasAnyFlags`.
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
Review Comment:
@manchurio You can omit `typedef` 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] manchurio commented on a diff in pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
manchurio commented on code in PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#discussion_r1006578545
##########
src/types/redis_zset.h:
##########
@@ -82,15 +82,39 @@ enum ZSetFlags {
kZSetXX = 1 << 2,
kZSetReversed = 1 << 3,
kZSetRemoved = 1 << 4,
+ kZSetGT = 1 << 5,
+ kZSetLT = 1 << 6,
+ kZSetCH = 1 << 7,
};
+typedef struct ZAddFlags {
+ explicit ZAddFlags(uint8_t flags = 0) : flags(flags) {}
+
+ bool HasNX() const { return (flags & kZSetNX) != 0; }
+ bool HasXX() const { return (flags & kZSetXX) != 0; }
+ bool HasLT() const { return (flags & kZSetLT) != 0; }
+ bool HasGT() const { return (flags & kZSetGT) != 0; }
+ bool HasCH() const { return (flags & kZSetCH) != 0; }
+ bool HasIncr() const { return (flags & kZSetIncr) != 0; }
+ bool HasFlag() const { return flags != 0; }
Review Comment:
Yes, you are right, I'd solved it later.
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1288054141
@manchurio You can also consider using something like the following to encapsulate the logic with options and eliminate code duplication:
```
struct ZAddOptions {
ZAddOptions() {}
explicit ZAddOptions(uint8_t flags) : flags(flags) {}
bool IsXX() const { return (flags & kZSetNX) != 0; }
// other methods to ask if the specific flag was set
uint8_t flags = 0;
};
```
Of course, you can add methods like `SetXX` to encapsulate set operations as well (to not spread bit operations over the codebase).
--
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 pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286956196
It is sad that cppcheck still fail and report a FALSE ALARM.
We will replace cppcheck by clang-tidy, and I think you can now workaround it by `auto iter = ...; if(iter != end()) ...`
cc @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] PragmaTwice commented on pull request #1022: Implement the ZADD options
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1022:
URL: https://github.com/apache/incubator-kvrocks/pull/1022#issuecomment-1286828998
You can just run `./x.py format` to format the source code rather than manual formatting
--
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