From 96dcd1183afa803d30ec71c574e80e3dc01c1f6e Mon Sep 17 00:00:00 2001 From: LiiNen Date: Wed, 29 May 2024 04:01:28 +0900 Subject: [PATCH] Change BITCOUNT 'end' as optional like BITPOS (#118) _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 Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com> --- src/bitops.c | 19 ++++++++++--------- src/commands.def | 18 ++++++++++++------ src/commands/bitcount.json | 37 ++++++++++++++++++++++++------------- tests/unit/bitops.tcl | 19 ++++++++++++++++--- 4 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/bitops.c b/src/bitops.c index db975e4dfe..7a385812d0 100644 --- a/src/bitops.c +++ b/src/bitops.c @@ -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 */ diff --git a/src/commands.def b/src/commands.def index f76e21f2f3..c59cb01dc1 100644 --- a/src/commands.def +++ b/src/commands.def @@ -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}, diff --git a/src/commands/bitcount.json b/src/commands/bitcount.json index 2d277a8551..ad90b39bad 100644 --- a/src/commands/bitcount.json +++ b/src/commands/bitcount.json @@ -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" + } + ] } ] } diff --git a/tests/unit/bitops.tcl b/tests/unit/bitops.tcl index edcafdee07..125b0a3d1f 100644 --- a/tests/unit/bitops.tcl +++ b/tests/unit/bitops.tcl @@ -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} }