git-hulk commented on code in PR #3475:
URL: https://github.com/apache/kvrocks/pull/3475#discussion_r3192636847
##########
src/types/redis_string.cc:
##########
@@ -249,6 +249,26 @@ rocksdb::Status String::Set(engine::Context &ctx, const
std::string &user_key, c
uint64_t old_expire = 0;
auto s = getValueAndExpire(ctx, ns_key, &old_value, &old_expire);
if (!s.ok() && !s.IsNotFound() && !s.IsInvalidArgument()) return s;
+ // If the existing key is not a string type, enforce expected behaviors:
+ if (s.IsInvalidArgument()) {
+ // For conditional comparisons (IFEQ/IFNE/IFDEQ/IFDNE), reading the old
value is required,
+ // so return the underlying WRONGTYPE (InvalidArgument) error.
+ if (args.type == StringSetType::IFEQ || args.type == StringSetType::IFNE
|| args.type == StringSetType::IFDEQ ||
+ args.type == StringSetType::IFDNE) {
+ return s;
+ }
+ // For NX option, treat a wrong type as "key exists" so the condition is
not met.
+ if (args.type == StringSetType::NX) {
+ // If GET is also specified, we need to return the WRONGTYPE error
+ // because GET requires reading the old value.
+ if (args.get) {
+ return s;
+ }
+ if (!args.get) ret = std::nullopt;
Review Comment:
Remove the unnecessary condition check: `if (!args.get)`
```suggestion
if (args.get) return s;
ret = std::nullopt;
```
##########
src/types/redis_string.cc:
##########
@@ -271,6 +291,38 @@ rocksdb::Status String::Set(engine::Context &ctx, const
std::string &user_key, c
// if XX option given, the key didn't exist before: return nil
if (!args.get) ret = std::nullopt;
return rocksdb::Status::OK();
+ } else if (args.type == StringSetType::IFEQ) {
+ // condition met only when key exists AND value matches
+ bool matched = s.ok() && (old_value == args.cmp_value);
+ if (!matched) {
+ if (!args.get) ret = std::nullopt;
+ return rocksdb::Status::OK();
+ }
+ if (!args.get) ret = "";
+ } else if (args.type == StringSetType::IFNE) {
+ // condition not met when key exists AND value matches; key-not-found
counts as met
+ bool not_matched = s.ok() && (old_value == args.cmp_value);
+ if (not_matched) {
+ if (!args.get) ret = std::nullopt;
+ return rocksdb::Status::OK();
+ }
+ if (!args.get) ret = "";
+ } else if (args.type == StringSetType::IFDEQ) {
+ // condition met only when key exists AND digest matches
(case-insensitive)
+ bool matched = s.ok() && util::EqualICase(util::StringDigest(old_value),
args.cmp_value);
+ if (!matched) {
+ if (!args.get) ret = std::nullopt;
+ return rocksdb::Status::OK();
+ }
+ if (!args.get) ret = "";
+ } else if (args.type == StringSetType::IFDNE) {
+ // condition not met when key exists AND digest matches
(case-insensitive); key-not-found counts as met
+ bool not_matched = s.ok() &&
util::EqualICase(util::StringDigest(old_value), args.cmp_value);
Review Comment:
The same issue with the above comment
##########
src/types/redis_string.cc:
##########
@@ -271,6 +291,38 @@ rocksdb::Status String::Set(engine::Context &ctx, const
std::string &user_key, c
// if XX option given, the key didn't exist before: return nil
if (!args.get) ret = std::nullopt;
return rocksdb::Status::OK();
+ } else if (args.type == StringSetType::IFEQ) {
+ // condition met only when key exists AND value matches
+ bool matched = s.ok() && (old_value == args.cmp_value);
+ if (!matched) {
+ if (!args.get) ret = std::nullopt;
+ return rocksdb::Status::OK();
+ }
+ if (!args.get) ret = "";
+ } else if (args.type == StringSetType::IFNE) {
+ // condition not met when key exists AND value matches; key-not-found
counts as met
+ bool not_matched = s.ok() && (old_value == args.cmp_value);
Review Comment:
the meaning of this result should be `matched`? instead of `not_matched`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
