slipenkomk/valkey
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Change BITCOUNT 'end' as optional like BITPOS (#118)

Browse Source 
_This change is the thing I suggested to redis when it was BSD, and is
not just migration - this is of course more advanced_

### Issue
There is weird difference in syntax between BITPOS and BITCOUNT:
```
BITPOS key bit [start [end [BYTE | BIT]]]
BITCOUNT key [start end [BYTE | BIT]]
```

I think this might cause confusion in terms of usability.
It was not just a syntax typo error, and really works differently.
The results below are with unstable build:
```
> get TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(error) ERR syntax error
```

### What did I fix

simply changes logic, to accept BITCOUNT also without 'end' - 'end'
become optional, like BITPOS
```
> GET TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(integer) 9
```

Of course, I also fixed syntax hint:
```
# ASIS 
> BITCOUNT key [start end [BYTE|BIT]]
# TOBE
> BITCOUNT key [start [end [BYTE|BIT]]]
```

![image](https://github.com/valkey-io/valkey/assets/38001238/8485f58e-6785-4106-9f3f-45e62f90d24b)


### Moreover ...
I hadn't noticed that there was very small dead code in these command
logic, when I wrote PR to redis.
I found it now, when write code again, so I wrote it in valkey.
``` c
/* asis unstable */

/* bitcountCommand() */
if (!strcasecmp(c->argv[4]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 4) {
    if (isbit) end = (totlen<<3) + 7;
    else end = totlen-1;
}

/* bitposCommand() */
if (!strcasecmp(c->argv[5]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 5) {
    if (isbit) end = (totlen<<3) + 7;
    else end = totlen-1;
}
```
Bit variable (actually int) "isbit" is only being set as 1, when 'BIT'
is declared.
But we were checking whether 'isbit' is true or false in this 'if'
phrase, even if isbit could never be 1, because argc is always less than
4 (or 5 in bitpos).



I think this minor fixes will make valkey command operation more
consistent.
Of course, this PR contains just changing args from "required" to
"optional", so it will never hurt previous users.

Thanks,

---------

Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
This commit is contained in:
LiiNen 2024-05-29 04:01:28 +09:00 committed by GitHub
parent fd58b73f0a
commit 96dcd1183a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 62 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/bitops.c
View File

@ -784,7 +784,7 @@ void bitopCommand(client *c) {
addReplyLongLong(c, maxlen); /* Return the output string length in bytes. */
}
/* BITCOUNT key [start end [BIT|BYTE]] */
/* BITCOUNT key [start [end [BIT|BYTE]]] */
void bitcountCommand(client *c) {
robj *o;
long long start, end;
@ -795,9 +795,8 @@ void bitcountCommand(client *c) {
unsigned char first_byte_neg_mask = 0, last_byte_neg_mask = 0;
/* Parse start/end range if any. */
if (c->argc == 4 || c->argc == 5) {
if (c->argc == 3 || c->argc == 4 || c->argc == 5) {
if (getLongLongFromObjectOrReply(c, c->argv[2], &start, NULL) != C_OK) return;
if (getLongLongFromObjectOrReply(c, c->argv[3], &end, NULL) != C_OK) return;
if (c->argc == 5) {
if (!strcasecmp(c->argv[4]->ptr, "bit"))
isbit = 1;
@ -808,6 +807,11 @@ void bitcountCommand(client *c) {
return;
}
}
if (c->argc >= 4) {
if (getLongLongFromObjectOrReply(c,c->argv[3],&end,NULL) != C_OK)
return;
}
/* Lookup, check for type. */
o = lookupKeyRead(c->db, c->argv[1]);
if (checkType(c, o, OBJ_STRING)) return;
@ -817,6 +821,8 @@ void bitcountCommand(client *c) {
/* Make sure we will not overflow */
serverAssert(totlen <= LLONG_MAX >> 3);
if (c->argc < 4) end = totlen-1;
/* Convert negative indexes */
if (start < 0 && end < 0 && start > end) {
addReply(c, shared.czero);
@ -921,12 +927,7 @@ void bitposCommand(client *c) {
long long totlen = strlen;
serverAssert(totlen <= LLONG_MAX >> 3);
if (c->argc < 5) {
if (isbit)
end = (totlen << 3) + 7;
else
end = totlen - 1;
}
if (c->argc < 5) end = totlen - 1;
if (isbit) totlen <<= 3;
/* Convert negative indexes */

18
src/commands.def
View File

@ -36,6 +36,7 @@ const char *commandGroupStr(int index) {
/* BITCOUNT history */
commandHistory BITCOUNT_History[] = {
{"7.0.0","Added the `BYTE|BIT` option."},
{"8.0.0","`end` made optional; when called without argument the command reports the last BYTE."},
};
#endif
@ -51,23 +52,28 @@ keySpec BITCOUNT_Keyspecs[1] = {
};
#endif
/* BITCOUNT range unit argument table */
struct COMMAND_ARG BITCOUNT_range_unit_Subargs[] = {
/* BITCOUNT range end_unit_block unit argument table */
struct COMMAND_ARG BITCOUNT_range_end_unit_block_unit_Subargs[] = {
{MAKE_ARG("byte",ARG_TYPE_PURE_TOKEN,-1,"BYTE",NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("bit",ARG_TYPE_PURE_TOKEN,-1,"BIT",NULL,NULL,CMD_ARG_NONE,0,NULL)},
};
/* BITCOUNT range end_unit_block argument table */
struct COMMAND_ARG BITCOUNT_range_end_unit_block_Subargs[] = {
{MAKE_ARG("end",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("unit",ARG_TYPE_ONEOF,-1,NULL,NULL,"7.0.0",CMD_ARG_OPTIONAL,2,NULL),.subargs=BITCOUNT_range_end_unit_block_unit_Subargs},
};
/* BITCOUNT range argument table */
struct COMMAND_ARG BITCOUNT_range_Subargs[] = {
{MAKE_ARG("start",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("end",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("unit",ARG_TYPE_ONEOF,-1,NULL,NULL,"7.0.0",CMD_ARG_OPTIONAL,2,NULL),.subargs=BITCOUNT_range_unit_Subargs},
{MAKE_ARG("end-unit-block",ARG_TYPE_BLOCK,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=BITCOUNT_range_end_unit_block_Subargs},
};
/* BITCOUNT argument table */
struct COMMAND_ARG BITCOUNT_Args[] = {
{MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
{MAKE_ARG("range",ARG_TYPE_BLOCK,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,3,NULL),.subargs=BITCOUNT_range_Subargs},
{MAKE_ARG("range",ARG_TYPE_BLOCK,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=BITCOUNT_range_Subargs},
};
/********** BITFIELD ********************/
@ -10661,7 +10667,7 @@ struct COMMAND_ARG WATCH_Args[] = {
/* Main command table */
struct COMMAND_STRUCT serverCommandTable[] = {
/* bitmap */
{MAKE_CMD("bitcount","Counts the number of set bits (population counting) in a string.","O(N)","2.6.0",CMD_DOC_NONE,NULL,NULL,"bitmap",COMMAND_GROUP_BITMAP,BITCOUNT_History,1,BITCOUNT_Tips,0,bitcountCommand,-2,CMD_READONLY,ACL_CATEGORY_BITMAP,BITCOUNT_Keyspecs,1,NULL,2),.args=BITCOUNT_Args},
{MAKE_CMD("bitcount","Counts the number of set bits (population counting) in a string.","O(N)","2.6.0",CMD_DOC_NONE,NULL,NULL,"bitmap",COMMAND_GROUP_BITMAP,BITCOUNT_History,2,BITCOUNT_Tips,0,bitcountCommand,-2,CMD_READONLY,ACL_CATEGORY_BITMAP,BITCOUNT_Keyspecs,1,NULL,2),.args=BITCOUNT_Args},
{MAKE_CMD("bitfield","Performs arbitrary bitfield integer operations on strings.","O(1) for each subcommand specified","3.2.0",CMD_DOC_NONE,NULL,NULL,"bitmap",COMMAND_GROUP_BITMAP,BITFIELD_History,0,BITFIELD_Tips,0,bitfieldCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_BITMAP,BITFIELD_Keyspecs,1,bitfieldGetKeys,2),.args=BITFIELD_Args},
{MAKE_CMD("bitfield_ro","Performs arbitrary read-only bitfield integer operations on strings.","O(1) for each subcommand specified","6.0.0",CMD_DOC_NONE,NULL,NULL,"bitmap",COMMAND_GROUP_BITMAP,BITFIELD_RO_History,0,BITFIELD_RO_Tips,0,bitfieldroCommand,-2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_BITMAP,BITFIELD_RO_Keyspecs,1,NULL,2),.args=BITFIELD_RO_Args},
{MAKE_CMD("bitop","Performs bitwise operations on multiple strings, and stores the result.","O(N)","2.6.0",CMD_DOC_NONE,NULL,NULL,"bitmap",COMMAND_GROUP_BITMAP,BITOP_History,0,BITOP_Tips,0,bitopCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_BITMAP,BITOP_Keyspecs,2,NULL,3),.args=BITOP_Args},

37
src/commands/bitcount.json
View File

@ -10,6 +10,10 @@
[
"7.0.0",
"Added the `BYTE|BIT` option."
],
[
"8.0.0",
"`end` made optional; when called without argument the command reports the last BYTE."
]
],
"command_flags": [
@ -54,24 +58,31 @@
"type": "integer"
},
{
"name": "end",
"type": "integer"
},
{
"name": "unit",
"type": "oneof",
"name": "end-unit-block",
"type": "block",
"optional": true,
"since": "7.0.0",
"arguments": [
{
"name": "byte",
"type": "pure-token",
"token": "BYTE"
"name": "end",
"type": "integer"
},
{
"name": "bit",
"type": "pure-token",
"token": "BIT"
"name": "unit",
"type": "oneof",
"optional": true,
"since": "7.0.0",
"arguments": [
{
"name": "byte",
"type": "pure-token",
"token": "BYTE"
},
{
"name": "bit",
"type": "pure-token",
"token": "BIT"
}
]
}
]
}

19
tests/unit/bitops.tcl
View File

@ -128,13 +128,26 @@ start_server {tags {"bitops"}} {
}
}
test {BITCOUNT with just start} {
set s "foobar"
r set s $s
assert_equal [r bitcount s 0] [count_bits "foobar"]
assert_equal [r bitcount s 1] [count_bits "oobar"]
assert_equal [r bitcount s 1000] 0
assert_equal [r bitcount s -1] [count_bits "r"]
assert_equal [r bitcount s -2] [count_bits "ar"]
assert_equal [r bitcount s -1000] [count_bits "foobar"]
}
test {BITCOUNT with start, end} {
set s "foobar"
r set s $s
assert_equal [r bitcount s 0 -1] [count_bits "foobar"]
assert_equal [r bitcount s 1 -2] [count_bits "ooba"]
assert_equal [r bitcount s -2 1] [count_bits ""]
assert_equal [r bitcount s -2 1] 0
assert_equal [r bitcount s -1000 0] [count_bits "f"]
assert_equal [r bitcount s 0 1000] [count_bits "foobar"]
assert_equal [r bitcount s -1000 1000] [count_bits "foobar"]
assert_equal [r bitcount s 0 -1 bit] [count_bits $s]
assert_equal [r bitcount s 10 14 bit] [count_bits_start_end $s 10 14]
@ -144,18 +157,18 @@ start_server {tags {"bitops"}} {
assert_equal [r bitcount s 3 -34 bit] [count_bits_start_end $s 3 14]
assert_equal [r bitcount s 3 -19 bit] [count_bits_start_end $s 3 29]
assert_equal [r bitcount s -2 1 bit] 0
assert_equal [r bitcount s -1000 14 bit] [count_bits_start_end $s 0 14]
assert_equal [r bitcount s 0 1000 bit] [count_bits $s]
assert_equal [r bitcount s -1000 1000 bit] [count_bits $s]
}
test {BITCOUNT with illegal arguments} {
# Used to return 0 for non-existing key instead of errors
r del s
assert_error {ERR *syntax*} {r bitcount s 0}
assert_error {ERR *syntax*} {r bitcount s 0 1 hello}
assert_error {ERR *syntax*} {r bitcount s 0 1 hello hello2}
r set s 1
assert_error {ERR *syntax*} {r bitcount s 0}
assert_error {ERR *syntax*} {r bitcount s 0 1 hello}
assert_error {ERR *syntax*} {r bitcount s 0 1 hello hello2}
}