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

Open
kirito632 wants to merge 3 commits into
base: unstable
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
71c4694
Merge branch 'unstable' into feat-set-ifeq
jihuayu Apr 15, 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
61 changes: 14 additions & 47 deletions src/commands/cmd_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,51 +108,6 @@ class CommandGetEx : public Commander {
std::optional<uint64_t> expire_;
};

class CommandDelEX : public Commander {
Copy link
Copy Markdown
Member

@jihuayu jihuayu Apr 15, 2026

Choose a reason for hiding this comment

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

Why are you remove it?

Copy link
Copy Markdown
Author

@kirito632 kirito632 Apr 15, 2026

Choose a reason for hiding this comment

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

@jihuayu Sorry for the confusion! I misunderstood your feedback about "splitting into 2-4 smaller PRs".

I initially thought you wanted completely separate PRs for SET and DelEX, so I created:

But now I realize you probably meant organizing the original PR #3393 into multiple clear commits, not separate PRs, since SET and DelEX share the same conditional logic (IFEQ/IFNE/IFDEQ/IFDNE).

What would you prefer?

Option A (Recommended): Close PR #3452 and #3453, go back to the original PR #3393, and reorganize it into clean commits:

  • Commit 1: Add conditional options infrastructure (IFEQ/IFNE/IFDEQ/IFDNE)
  • Commit 2: Implement SET with conditional options
  • Commit 3: Implement DelEX command
  • Commit 4: Add Go integration tests
  • Commit 5: Add test case for SET NX GET on wrong type

Option B: Keep SET and DelEX as separate PRs, but make DelEX depend on SET being merged first.

Please let me know which approach you prefer, and I'll fix it right away. Thanks for your patience!

public:
Status Parse(const std::vector<std::string> &args) override {
if (args.size() > 4) {
return {Status::RedisParseErr, errWrongNumOfArguments};
}

CommandParser parser(args, 2);
while (parser.Good()) {
if (parser.EatEqICase("ifdeq")) {
option_ = {DelExOption::IFDEQ, GET_OR_RET(parser.TakeStr())};
} else if (parser.EatEqICase("ifdne")) {
option_ = {DelExOption::IFDNE, GET_OR_RET(parser.TakeStr())};
} else if (parser.EatEqICase("ifeq")) {
option_ = {DelExOption::IFEQ, GET_OR_RET(parser.TakeStr())};
} else if (parser.EatEqICase("ifne")) {
option_ = {DelExOption::IFNE, GET_OR_RET(parser.TakeStr())};
} else {
return {Status::RedisParseErr, errInvalidSyntax};
}
}
return Status::OK();
}

Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override {
redis::String string_db(srv->storage, conn->GetNamespace());
bool deleted = false;
auto s = string_db.DelEX(ctx, args_[1], option_, deleted);

if (!s.ok() && !s.IsNotFound()) {
return {Status::RedisExecErr, s.ToString()};
}

if (s.IsNotFound() || !deleted) {
*output = redis::Integer(0);
} else {
*output = redis::Integer(1);
}
return Status::OK();
}

private:
DelExOption option_;
};

class CommandStrlen : public Commander {
public:
Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override {
Expand Down Expand Up @@ -344,6 +299,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 +325,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 +352,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 Expand Up @@ -857,7 +825,6 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandGetRange>("getrange", 4, "read-only", 1, 1, 1),
MakeCmdAttr<CommandSubStr>("substr", 4, "read-only", 1, 1, 1),
MakeCmdAttr<CommandGetDel>("getdel", 2, "write no-dbsize-check", 1, 1, 1),
MakeCmdAttr<CommandDelEX>("delex", -2, "write", 1, 1, 1),
MakeCmdAttr<CommandSetRange>("setrange", 4, "write", 1, 1, 1),
MakeCmdAttr<CommandMGet>("mget", -2, "read-only", 1, -1, 1),
MakeCmdAttr<CommandAppend>("append", 3, "write", 1, 1, 1), MakeCmdAttr<CommandSet>("set", -3, "write", 1, 1, 1),
Expand Down
84 changes: 48 additions & 36 deletions src/types/redis_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,41 +183,6 @@ rocksdb::Status String::GetEx(engine::Context &ctx, const std::string &user_key,
return rocksdb::Status::OK();
}

rocksdb::Status String::DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option,
bool &deleted) {
deleted = false;
std::string val;
std::string ns_key = AppendNamespacePrefix(user_key);
rocksdb::Status s = getValue(ctx, ns_key, &val);
if (!s.ok()) return s;

bool matched = false;
switch (option.type) {
case DelExOption::NONE:
matched = true;
break;
case DelExOption::IFDEQ:
matched = option.value == util::StringDigest(val);
break;
case DelExOption::IFDNE:
matched = option.value != util::StringDigest(val);
break;
case DelExOption::IFEQ:
matched = option.value == val;
break;
case DelExOption::IFNE:
matched = option.value != val;
break;
default:
return rocksdb::Status::InvalidArgument();
}
if (matched) {
s = storage_->Delete(ctx, storage_->DefaultWriteOptions(), metadata_cf_handle_, ns_key);
deleted = s.ok();
}
return s;
}

rocksdb::Status String::GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value,
std::optional<std::string> &old_value) {
auto s =
Expand All @@ -239,7 +204,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,
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 +214,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 +251,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
17 changes: 4 additions & 13 deletions src/types/redis_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,15 @@ struct StringPair {
Slice value;
};

struct DelExOption {
enum Type { NONE, IFDEQ, IFDNE, IFEQ, IFNE };
Type type;
std::string value;

DelExOption() : type(NONE) {}
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 @@ -98,13 +90,12 @@ class String : public Database {
rocksdb::Status Get(engine::Context &ctx, const std::string &user_key, std::string *value);
rocksdb::Status GetEx(engine::Context &ctx, const std::string &user_key, std::string *value,
std::optional<uint64_t> expire);
rocksdb::Status DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option, bool &deleted);
rocksdb::Status GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value,
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
Loading
Loading