Skip to content
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

Jump to bottom

Set Command with IFEQ Support #1324

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from
+149 −34
Open

Set Command with IFEQ Support #1324

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4a36392
set with ifeq support
sarthakaggarwal97 Nov 19, 2024
b7ed294
addressing comments and adding tests
sarthakaggarwal97 Nov 20, 2024
8b013ed
fixes
sarthakaggarwal97 Nov 20, 2024
fa789e6
addressing comments
sarthakaggarwal97 Nov 20, 2024
07f0fd5
addressing tests
sarthakaggarwal97 Nov 20, 2024
f796577
clang check
sarthakaggarwal97 Nov 20, 2024
f5ec47c
remove dedup lookup
sarthakaggarwal97 Nov 20, 2024
3dc3407
addressing comments
sarthakaggarwal97 Nov 20, 2024
62b8832
addressing comments
sarthakaggarwal97 Nov 25, 2024
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
3 changes: 2 additions & 1 deletion src/commands.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10666,6 +10666,7 @@ struct COMMAND_ARG SET_expiration_Subargs[] = {
struct COMMAND_ARG SET_Args[] = {
{MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("value",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("comparison-value",ARG_TYPE_STRING,-1,"IFEQ","Sets the key's value only if the current value matches the specified comparison value.",NULL,CMD_ARG_OPTIONAL,0,NULL)},
{MAKE_ARG("condition",ARG_TYPE_ONEOF,-1,NULL,NULL,"2.6.12",CMD_ARG_OPTIONAL,2,NULL),.subargs=SET_condition_Subargs},
{MAKE_ARG("get",ARG_TYPE_PURE_TOKEN,-1,"GET",NULL,"6.2.0",CMD_ARG_OPTIONAL,0,NULL)},
{MAKE_ARG("expiration",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,5,NULL),.subargs=SET_expiration_Subargs},
Expand Down Expand Up @@ -11139,7 +11140,7 @@ struct COMMAND_STRUCT serverCommandTable[] = {
{MAKE_CMD("mset","Atomically creates or modifies the string values of one or more keys.","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,MSET_History,0,MSET_Tips,2,msetCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,MSET_Keyspecs,1,NULL,1),.args=MSET_Args},
{MAKE_CMD("msetnx","Atomically modifies the string values of one or more keys only when all keys don't exist.","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,MSETNX_History,0,MSETNX_Tips,0,msetnxCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,MSETNX_Keyspecs,1,NULL,1),.args=MSETNX_Args},
{MAKE_CMD("psetex","Sets both string value and expiration time in milliseconds of a key. The key is created if it doesn't exist.","O(1)","2.6.0",CMD_DOC_DEPRECATED,"`SET` with the `PX` argument","2.6.12","string",COMMAND_GROUP_STRING,PSETEX_History,0,PSETEX_Tips,0,psetexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,PSETEX_Keyspecs,1,NULL,3),.args=PSETEX_Args},
{MAKE_CMD("set","Sets the string value of a key, ignoring its type. The key is created if it doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SET_History,4,SET_Tips,0,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SET_Keyspecs,1,setGetKeys,5),.args=SET_Args},
{MAKE_CMD("set","Sets the string value of a key, ignoring its type. The key is created if it doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SET_History,4,SET_Tips,0,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SET_Keyspecs,1,setGetKeys,6),.args=SET_Args},
{MAKE_CMD("setex","Sets the string value and expiration time of a key. Creates the key if it doesn't exist.","O(1)","2.0.0",CMD_DOC_DEPRECATED,"`SET` with the `EX` argument","2.6.12","string",COMMAND_GROUP_STRING,SETEX_History,0,SETEX_Tips,0,setexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SETEX_Keyspecs,1,NULL,3),.args=SETEX_Args},
{MAKE_CMD("setnx","Set the string value of a key only when the key doesn't exist.","O(1)","1.0.0",CMD_DOC_DEPRECATED,"`SET` with the `NX` argument","2.6.12","string",COMMAND_GROUP_STRING,SETNX_History,0,SETNX_Tips,0,setnxCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,SETNX_Keyspecs,1,NULL,2),.args=SETNX_Args},
{MAKE_CMD("setrange","Overwrites a part of a string value with another by an offset. Creates the key if it doesn't exist.","O(1), not counting the time taken to copy the new string in place. Usually, this string is very small so the amortized complexity is O(1). Otherwise, complexity is O(M) with M being the length of the value argument.","2.2.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SETRANGE_History,0,SETRANGE_Tips,0,setrangeCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SETRANGE_Keyspecs,1,NULL,3),.args=SETRANGE_Args},
Expand Down
14 changes: 14 additions & 0 deletions src/commands/set.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,20 @@
"name": "value",
"type": "string"
},
{
"name": "comparison-value",
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
"type": "string",
"token": "IFEQ",
"optional": true,
"summary": "Sets the key's value only if the current value matches the specified comparison value.",
"arguments": [
{
"name": "comparison-value",
"type": "string",
"summary": "The value to compare with the current key's value before setting."
}
]
},
{
"name": "condition",
"type": "oneof",
Expand Down
83 changes: 61 additions & 22 deletions src/t_string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,16 @@ static int checkStringLength(client *c, long long size, long long append) {
* If abort_reply is NULL, "$-1" is used. */

#define OBJ_NO_FLAGS 0
#define OBJ_SET_NX (1 << 0) /* Set if key not exists. */
#define OBJ_SET_XX (1 << 1) /* Set if key exists. */
#define OBJ_EX (1 << 2) /* Set if time in seconds is given */
#define OBJ_PX (1 << 3) /* Set if time in ms in given */
#define OBJ_KEEPTTL (1 << 4) /* Set and keep the ttl */
#define OBJ_SET_GET (1 << 5) /* Set if want to get key before set */
#define OBJ_EXAT (1 << 6) /* Set if timestamp in second is given */
#define OBJ_PXAT (1 << 7) /* Set if timestamp in ms is given */
#define OBJ_PERSIST (1 << 8) /* Set if we need to remove the ttl */
#define OBJ_SET_NX (1 << 0) /* Set if key not exists. */
#define OBJ_SET_XX (1 << 1) /* Set if key exists. */
#define OBJ_EX (1 << 2) /* Set if time in seconds is given */
#define OBJ_PX (1 << 3) /* Set if time in ms in given */
#define OBJ_KEEPTTL (1 << 4) /* Set and keep the ttl */
#define OBJ_SET_GET (1 << 5) /* Set if want to get key before set */
#define OBJ_EXAT (1 << 6) /* Set if timestamp in second is given */
#define OBJ_PXAT (1 << 7) /* Set if timestamp in ms is given */
#define OBJ_PERSIST (1 << 8) /* Set if we need to remove the ttl */
#define OBJ_SET_IFEQ (1 << 9) /* Set if we need compare and set */

/* Forward declaration */
static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int unit, long long *milliseconds);
Expand All @@ -87,7 +88,8 @@ void setGenericCommand(client *c,
robj *expire,
int unit,
robj *ok_reply,
robj *abort_reply) {
robj *abort_reply,
robj *comparison) {
long long milliseconds = 0; /* initialized to avoid any harmness warning */
int found = 0;
int setkey_flags = 0;
Expand All @@ -100,7 +102,32 @@ void setGenericCommand(client *c,
if (getGenericCommand(c) == C_ERR) return;
}

found = (lookupKeyWrite(c->db, key) != NULL);
const robj *value = lookupKeyWrite(c->db, key);
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
found = value != NULL;

/* Handle the IFEQ conditional check */
if (flags & OBJ_SET_IFEQ && found) {

sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
if (value->type != OBJ_STRING) {
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
if (!(flags & OBJ_SET_GET)) {
addReplyError(c, "value(s) must be present or string");
}
return;
}

if (compareStringObjects(value, comparison) != 0) {
if (!(flags & OBJ_SET_GET)) {
addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
}
return;
}

} else if (flags & OBJ_SET_IFEQ && !found) {
if (!(flags & OBJ_SET_GET)) {
addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
}
return;
}

if ((flags & OBJ_SET_NX && found) || (flags & OBJ_SET_XX && !found)) {
if (!(flags & OBJ_SET_GET)) {
Expand Down Expand Up @@ -208,7 +235,7 @@ static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int
* string arguments used in SET and GET command.
*
* Get specific commands - PERSIST/DEL
* Set specific commands - XX/NX/GET
* Set specific commands - XX/NX/GET/IFEQ
* Common commands - EX/EXAT/PX/PXAT/KEEPTTL
*
* Function takes pointers to client, flags, unit, pointer to pointer of expire obj if needed
Expand All @@ -219,7 +246,7 @@ static int getExpireMillisecondsOrReply(client *c, robj *expire, int flags, int
* Input flags are updated upon parsing the arguments. Unit and expire are updated if there are any
* EX/EXAT/PX/PXAT arguments. Unit is updated to millisecond if PX/PXAT is set.
*/
int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj **expire, int command_type) {
int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj **expire, robj **compare_val, int command_type) {
int j = command_type == COMMAND_GET ? 2 : 3;
for (; j < c->argc; j++) {
char *opt = c->argv[j]->ptr;
Expand Down Expand Up @@ -295,7 +322,17 @@ int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj *
*unit = UNIT_MILLISECONDS;
*expire = next;
j++;
} else {
} else if ((opt[0] == 'i' || opt[0] == 'I') &&
(opt[1] == 'f' || opt[1] == 'F') &&
(opt[2] == 'e' || opt[2] == 'E') &&
(opt[3] == 'q' || opt[3] == 'Q') && opt[4] == '\0' &&
next && (command_type == COMMAND_SET))
{
*flags |= OBJ_SET_IFEQ;
*compare_val = next;
j++;
}
else {
addReplyErrorObject(c,shared.syntaxerr);
return C_ERR;
}
Expand All @@ -304,34 +341,35 @@ int parseExtendedStringArgumentsOrReply(client *c, int *flags, int *unit, robj *
return C_OK;
}

/* SET key value [NX] [XX] [KEEPTTL] [GET] [EX <seconds>] [PX <milliseconds>]
* [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] */
/* SET key value [IFEQ <comparison-value>] [NX] [XX] [KEEPTTL] [GET] [EX <seconds>]
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
* [PX <milliseconds>] [EXAT <seconds-timestamp>][PXAT <milliseconds-timestamp>] */
void setCommand(client *c) {
robj *expire = NULL;
robj *comparison = NULL;
int unit = UNIT_SECONDS;
int flags = OBJ_NO_FLAGS;

if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, COMMAND_SET) != C_OK) {
if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_SET) != C_OK) {
return;
}

c->argv[2] = tryObjectEncoding(c->argv[2]);
setGenericCommand(c, flags, c->argv[1], c->argv[2], expire, unit, NULL, NULL);
setGenericCommand(c, flags, c->argv[1], c->argv[2], expire, unit, NULL, NULL, comparison);
}

void setnxCommand(client *c) {
c->argv[2] = tryObjectEncoding(c->argv[2]);
setGenericCommand(c, OBJ_SET_NX, c->argv[1], c->argv[2], NULL, 0, shared.cone, shared.czero);
setGenericCommand(c, OBJ_SET_NX, c->argv[1], c->argv[2], NULL, 0, shared.cone, shared.czero, NULL);
}

void setexCommand(client *c) {
c->argv[3] = tryObjectEncoding(c->argv[3]);
setGenericCommand(c, OBJ_EX, c->argv[1], c->argv[3], c->argv[2], UNIT_SECONDS, NULL, NULL);
setGenericCommand(c, OBJ_EX, c->argv[1], c->argv[3], c->argv[2], UNIT_SECONDS, NULL, NULL, NULL);
}

void psetexCommand(client *c) {
c->argv[3] = tryObjectEncoding(c->argv[3]);
setGenericCommand(c, OBJ_PX, c->argv[1], c->argv[3], c->argv[2], UNIT_MILLISECONDS, NULL, NULL);
setGenericCommand(c, OBJ_PX, c->argv[1], c->argv[3], c->argv[2], UNIT_MILLISECONDS, NULL, NULL, NULL);
}

int getGenericCommand(client *c) {
Expand Down Expand Up @@ -374,10 +412,11 @@ void getCommand(client *c) {
*/
void getexCommand(client *c) {
robj *expire = NULL;
robj *comparison = NULL;
int unit = UNIT_SECONDS;
int flags = OBJ_NO_FLAGS;

if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, COMMAND_GET) != C_OK) {
if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IFEQ is not allowed with COMMAND_GET, so I think we can just pass NULL for the comparison argument. It will not be used. Makes sense?

Suggested change
if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, &comparison, COMMAND_GET) != C_OK) {
if (parseExtendedStringArgumentsOrReply(c, &flags, &unit, &expire, NULL, COMMAND_GET) != C_OK) {

return;
}

Expand Down
24 changes: 12 additions & 12 deletions tests/assets/test_cli_hint_suite.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,18 @@
"ZRANGE k 1 2 WITHSCORES " "[BYSCORE|BYLEX] [REV] [LIMIT offset count]"

# Optional one-of args with parameters: SET key value [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]
"SET key value " "[NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EX" "[NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EX " "seconds [NX|XX] [GET]"
"SET key value EX 23 " "[NX|XX] [GET]"
"SET key value EXAT" "[NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EXAT " "unix-time-seconds [NX|XX] [GET]"
"SET key value PX" "[NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value PX " "milliseconds [NX|XX] [GET]"
"SET key value PXAT" "[NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value PXAT " "unix-time-milliseconds [NX|XX] [GET]"
"SET key value KEEPTTL " "[NX|XX] [GET]"
"SET key value XX " "[GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value " "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these...

Suggested change
"SET key value EX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EX" "[NX|XX|IFEQ comparison-value]] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"

"SET key value EX " "seconds [IFEQ comparison-value] [NX|XX] [GET]"
"SET key value EX 23 " "[IFEQ comparison-value] [NX|XX] [GET]"
"SET key value EXAT" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value EXAT " "unix-time-seconds [IFEQ comparison-value] [NX|XX] [GET]"
"SET key value PX" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value PX " "milliseconds [IFEQ comparison-value] [NX|XX] [GET]"
"SET key value PXAT" "[IFEQ comparison-value] [NX|XX] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"
"SET key value PXAT " "unix-time-milliseconds [IFEQ comparison-value] [NX|XX] [GET]"
"SET key value KEEPTTL " "[IFEQ comparison-value] [NX|XX] [GET]"
"SET key value XX " "[IFEQ comparison-value] [GET] [EX seconds|PX milliseconds|EXAT unix-time-seconds|PXAT unix-time-milliseconds|KEEPTTL]"

# If an input word can't be matched, stop hinting.
"SET key value FOOBAR " ""
Expand Down
53 changes: 53 additions & 0 deletions tests/unit/type/string.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,59 @@ if {[string match {*jemalloc*} [s mem_allocator]]} {
set err1
} {*WRONGTYPE*}

test "SET with IFEQ conditional" {
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved
r del foo

r set foo "initial_value"

assert_equal {OK} [r set foo "new_value" ifeq "initial_value"]
assert_equal "new_value" [r get foo]

assert_equal {} [r set foo "should_not_set" ifeq "wrong_value"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@valkey-io/core-team On failure of compare/set instead of nil value we should return an error with old value in it. Otherwise, a client would need to perform another GET operation.

Copy link
Member

@enjoy-binbin enjoy-binbin Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

return an error with old value seems odd to me. i think in normal cases, a client is unlikely get an error in set IFEQ, they hold the old value in somewhere, and if the old value is unvalid, this mean client should abort the set, this usually means the client should abort the entire business logic. In this case, client should GET the new value as needed and usually they don't really need the new value, they juse want the result of whether the SET succeeded or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@enjoy-binbin I understand if the value is updated from a DB to Valkey as a cache, the client won't benefit much from the value present.

However, if I think Valkey being used as a datastore and value is updated by multiple clients based on the value stored (let say increment old value by 1). for this case, they would need to know the previous value.

Let's hear other devs opinion on this and resolve this.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of returning value if the IFEQ fails, not as an error, just as a string. That way you can also check to see if someone else did the same work you did (and silently succeed) or retry you work again.

assert_equal "new_value" [r get foo]
}

test "SET with IFEQ conditional - non-string current value" {
r del foo

r sadd foo "some_set_value"
assert_error {ERR value(s) must be present or string} {r set foo "new_value" ifeq "some_set_value"}
}
sarthakaggarwal97 marked this conversation as resolved.
Show resolved Hide resolved

test "SET with IFEQ conditional - with get" {
r del foo

assert_equal {} [r set foo "new_value" ifeq "initial_value" get]

r set foo "initial_value"

assert_equal "initial_value" [r set foo "new_value" ifeq "initial_value" get]
assert_equal "new_value" [r get foo]
}


test "SET with IFEQ conditional - with xx" {
r del foo

assert_equal {} [r set foo "new_value" ifeq "initial_value" xx]

r set foo "initial_value"

assert_equal {OK} [r set foo "new_value" ifeq "initial_value" xx]
assert_equal "new_value" [r get foo]
}

test "SET with IFEQ conditional - with nx" {
r del foo

assert_equal {} [r set foo "new_value" ifeq "initial_value" nx]

r set foo "initial_value"

assert_equal {} [r set foo "new_value" ifeq "initial_value" nx]
assert_equal "initial_value" [r get foo]
}

test {Extended SET EX option} {
r del foo
r set foo bar ex 10
Expand Down
Loading