Skip to content

Commit

Permalink
Change BITCOUNT 'end' as optional like BITPOS (#118)
Browse files Browse the repository at this point in the history 
_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>
  • Loading branch information
@LiiNen @madolson
LiiNen and madolson authored May 28, 2024
1 parent fd58b73 commit 96dcd11
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 31 deletions.
19 changes: 10 additions & 9 deletions src/bitops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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 */
Expand Down
18 changes: 12 additions & 6 deletions src/commands.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 ********************/
Expand Down Expand Up @@ -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},
Expand Down
37 changes: 24 additions & 13 deletions src/commands/bitcount.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down Expand Up @@ -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"
}
]
}
]
}
Expand Down
19 changes: 16 additions & 3 deletions tests/unit/bitops.tcl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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}
}
Expand Down

0 comments on commit 96dcd11

Please sign in to comment.