Skip to content

feat(string): support IFEQ/IFNE/IFDEQ/IFDNE conditions for SET command #3452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kirito632 wants to merge 10 commits into from
Closed

feat(string): support IFEQ/IFNE/IFDEQ/IFDNE conditions for SET command #3452

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fbd986c
feat(string): support IFEQ/IFNE/IFDEQ/IFDNE conditions for SET command
kirito632 Apr 15, 2026
eb4f468
test(cppunit): remove DelEX unit tests from SET PR
kirito632 Apr 15, 2026
0ba1990
feat(string): support IFEQ/IFNE/IFDEQ/IFDNE in SET command
kirito632 Apr 16, 2026
614b188
Merge branch 'unstable' into feat-set-ifeq
jihuayu Apr 17, 2026
8e46585
Merge branch 'unstable' into feat-set-ifeq
jihuayu Apr 17, 2026
b22088c
fix(string): handle uppercase digest and fix NX+GET wrong type behavior
kirito632 Apr 17, 2026
aa8e0e3
Merge branch 'origin/feat-set-ifeq' with local fixes for uppercase di…
kirito632 Apr 17, 2026
cc078ed
Merge branch 'unstable' into feat-set-ifeq
jihuayu Apr 17, 2026
8a02c75
test(string): remove isolated DelEX tests from SET PR
kirito632 Apr 17, 2026
5757c3b
Merge branch 'feat-set-ifeq' of https://github.com/kirito632/kvrocks …
kirito632 Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/commands/cmd_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,18 @@ class CommandSet : public Commander {
set_flag_ = StringSetType::NX;
} else if (parser.EatEqICaseFlag("XX", set_flag)) {
set_flag_ = StringSetType::XX;
} else if (parser.EatEqICaseFlag("IFEQ", set_flag)) {
set_flag_ = StringSetType::IFEQ;
cmp_value_ = GET_OR_RET(parser.TakeStr());
} else if (parser.EatEqICaseFlag("IFNE", set_flag)) {
set_flag_ = StringSetType::IFNE;
cmp_value_ = GET_OR_RET(parser.TakeStr());
} else if (parser.EatEqICaseFlag("IFDEQ", set_flag)) {
set_flag_ = StringSetType::IFDEQ;
cmp_value_ = GET_OR_RET(parser.TakeStr());
} else if (parser.EatEqICaseFlag("IFDNE", set_flag)) {
set_flag_ = StringSetType::IFDNE;
cmp_value_ = GET_OR_RET(parser.TakeStr());
} else if (parser.EatEqICase("GET")) {
get_ = true;
} else {
Expand All @@ -358,7 +370,7 @@ class CommandSet : public Commander {
std::optional<std::string> ret;
redis::String string_db(srv->storage, conn->GetNamespace());

rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_}, ret);
rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_, cmp_value_}, ret);

if (!s.ok()) {
return {Status::RedisExecErr, s.ToString()};
Expand All @@ -385,6 +397,7 @@ class CommandSet : public Commander {
bool get_ = false;
bool keep_ttl_ = false;
StringSetType set_flag_ = StringSetType::NONE;
std::string cmp_value_;
};

class CommandSetEX : public Commander {
Expand Down
49 changes: 48 additions & 1 deletion src/types/redis_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c
}

rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value,

@jihuayu jihuayu Apr 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is getting a bit complex. You could try refactoring it in this PR, or open a new PR to refactor it after this one is merged.

@kirito632 kirito632 Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a new commit that addresses all of them:
Uppercase & Malformed Digests: Added the tests and updated the C++ logic (util::EqualICase and length validation) to handle them correctly.
NX + GET on Wrong Type: I found that the original C++ logic was short-circuiting and returning nil when NX failed, bypassing the GET type check. I have fixed this in redis_string.cc so it now properly returns a WRONGTYPE error, exactly matching Redis standard behavior. The tests pass beautifully now.
Regarding the String::Set refactoring: to keep this PR focused and safe, I will open a separate PR to refactor that function after this one is merged.

StringSetArgs args, std::optional<std::string> &ret) {
const StringSetArgs &args, std::optional<std::string> &ret) {
uint64_t expire = 0;
std::string ns_key = AppendNamespacePrefix(user_key);

Expand All @@ -249,6 +249,21 @@ 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 (!args.get) ret = std::nullopt;
return rocksdb::Status::OK();
}
// For other options, continue (e.g., XX may still proceed since key exists).
}
// GET option
if (args.get) {
if (s.IsInvalidArgument()) {
Expand All @@ -271,6 +286,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
bool matched = s.ok() && (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; key-not-found counts as met
bool not_matched = s.ok() && (util::StringDigest(old_value) == args.cmp_value);
if (not_matched) {
if (!args.get) ret = std::nullopt;
return rocksdb::Status::OK();
}
if (!args.get) ret = "";
} else {
// if GET option not given, make ret not nil
if (!args.get) ret = "";
Expand Down
7 changes: 4 additions & 3 deletions src/types/redis_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,15 @@ struct DelExOption {
DelExOption(Type type, std::string value) : type(type), value(std::move(value)) {}
};

enum class StringSetType { NONE, NX, XX };
enum class StringSetType { NONE, NX, XX, IFEQ, IFNE, IFDEQ, IFDNE };

struct StringSetArgs {
// Expire time in mill seconds.
uint64_t expire;
StringSetType type;
bool get;
bool keep_ttl;
std::string cmp_value; // valid only when type is IFEQ/IFNE/IFDEQ/IFDNE
};

struct StringMSetArgs {
Expand Down Expand Up @@ -103,8 +104,8 @@ class String : public Database {
std::optional<std::string> &old_value);
rocksdb::Status GetDel(engine::Context &ctx, const std::string &user_key, std::string *value);
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value);
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value, StringSetArgs args,
std::optional<std::string> &ret);
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value,
const StringSetArgs &args, std::optional<std::string> &ret);
rocksdb::Status SetEX(engine::Context &ctx, const std::string &user_key, const std::string &value,
uint64_t expire_ms);
rocksdb::Status SetNX(engine::Context &ctx, const std::string &user_key, const std::string &value, uint64_t expire_ms,
Expand Down
43 changes: 43 additions & 0 deletions tests/cppunit/types/string_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,3 +613,46 @@ TEST_F(RedisStringTest, LCS) {
4},
std::get<StringLCSIdxResult>(rst));
}

TEST_F(RedisStringTest, SetIFDEQ) {
std::string key = "ifdeq-key";
std::string value = "hello";
std::optional<std::string> ret;

// key not found → condition not met, no write
auto s = string_->Set(*ctx_, key, "new", {0, StringSetType::IFDEQ, false, false, util::StringDigest(value)}, ret);
EXPECT_TRUE(s.ok());
EXPECT_FALSE(ret.has_value());
std::string got;
EXPECT_TRUE(string_->Get(*ctx_, key, &got).IsNotFound());

// set up the key
string_->Set(*ctx_, key, value);

// digest matches → write succeeds
ret = std::nullopt;
s = string_->Set(*ctx_, key, "new", {0, StringSetType::IFDEQ, false, false, util::StringDigest(value)}, ret);
EXPECT_TRUE(s.ok());
EXPECT_TRUE(ret.has_value());
string_->Get(*ctx_, key, &got);
EXPECT_EQ("new", got);

// digest mismatches → no write
ret = std::nullopt;
s = string_->Set(*ctx_, key, "newer", {0, StringSetType::IFDEQ, false, false, "xxxxxxxxxxxxxxxx"}, ret);
EXPECT_TRUE(s.ok());
EXPECT_FALSE(ret.has_value());
string_->Get(*ctx_, key, &got);
EXPECT_EQ("new", got);

// empty string edge case: digest of "" is well-defined
string_->Set(*ctx_, key, "");
ret = std::nullopt;
s = string_->Set(*ctx_, key, "nonempty", {0, StringSetType::IFDEQ, false, false, util::StringDigest("")}, ret);
EXPECT_TRUE(s.ok());
EXPECT_TRUE(ret.has_value());
string_->Get(*ctx_, key, &got);
EXPECT_EQ("nonempty", got);

EXPECT_TRUE(string_->Del(*ctx_, key).ok());
}
Loading
Loading