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

Change BITCOUNT 'end' as optional like BITPOS #118

Merged
merged 12 commits into from
May 28, 2024
Merged

Change BITCOUNT 'end' as optional like BITPOS #118

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a6f4296
Fix to allow end as optional when using bitcount
LiiNen Apr 1, 2024
5b067d3
Fix hint for bitcount
LiiNen Apr 1, 2024
8e813f3
Add test in case for only using start in bitcount
LiiNen Apr 1, 2024
64008bb
Remove unreachable code in BITCOUNT and BITPOS
LiiNen Apr 1, 2024
fb2e403
Move line end = totlen - 1, after totlen declared
LiiNen Apr 1, 2024
c758112
Add commands.def after build
LiiNen Apr 1, 2024
fc369bb
Add history for this fix
LiiNen Apr 2, 2024
d5f0da6
Add commands.def after build
LiiNen Apr 2, 2024
729ec8b
Update tests/unit/bitops.tcl
LiiNen Apr 2, 2024
c0dd4ee
Fix test code : allowing bitcount without end now
LiiNen Apr 2, 2024
843c84a
Add test cases; larger value, negative larger value
LiiNen Apr 29, 2024
153625c
Merge branch 'unstable' into bitcount-end-as-optional
LiiNen May 27, 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
19 changes: 11 additions & 8 deletions src/bitops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,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 @@ -803,11 +803,9 @@ 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;
else if (!strcasecmp(c->argv[4]->ptr,"byte")) isbit = 0;
Expand All @@ -816,6 +814,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 @@ -825,6 +828,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 All @@ -849,6 +854,7 @@ void bitcountCommand(client *c) {
o = lookupKeyRead(c->db, c->argv[1]);
if (checkType(c, o, OBJ_STRING)) return;
p = getObjectReadOnlyString(o,&strlen,llbuf);

/* The whole string. */
start = 0;
end = strlen-1;
Expand Down Expand Up @@ -930,10 +936,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
Open in desktop
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 @@ -10645,7 +10651,7 @@ struct COMMAND_ARG WATCH_Args[] = {
/* Main command table */
struct COMMAND_STRUCT redisCommandTable[] = {
/* 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
Open in desktop
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",
madolson marked this conversation as resolved.
Show resolved Hide resolved
"type": "block",
"optional": true,
"since": "7.0.0",
"arguments": [
{
"name": "byte",
"type": "pure-token",
"token": "BYTE"
"name": "end",
"type": "integer"
Copy link
Contributor Author

@LiiNen LiiNen Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

end is in optional block, so this is useless I think.
I saw syntax again and below is what I have changed.

BITPOS key bit [start [end [BYTE | BIT]]]
BITCOUNT key [start end [BYTE | BIT]] -> BITCOUNT key [start [end [BYTE | BIT]]]

If end is also optional, then the syntax should be like below:

BITCOUNT key [start [[end] [BYTE | BIT]]]

I didn't think it before, and it seems this is also maybe reasonable - allowing using just 'start' and '[BYTE | BIT]'.
But in this case, it is harmful for backwards compatibility, and maybe critical to client libraries.

ex. BITCOUNT key start BYTE will be think as BITCOUNT key start end in client libraries, and will be return error because end is just string 'BYTE'.

It will be a quiet issue. Is this what you meant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the one I am wondering..
argument end was existed before, and this change is just making it as optional. since is also needed in this case?

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Just ignore my suggestion code changes and keep current json file.
For this PR, just add more several edge cases and I will approve it, Thanks

},
{
"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
11 changes: 9 additions & 2 deletions tests/unit/bitops.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,15 @@ 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 -1] [count_bits "r"]
assert_equal [r bitcount s -2] [count_bits "ar"]
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add several special cases which include larger value of start, such as start = 10 or 50, Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some new test cases in near days. thxs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more testcases; 843c84a

  1. Larger value of start as you said
  2. Larger negative value of start.
  3. Also added large negative value in BITCOUNT with start, end
  4. fix [count_bits ""] -> 0, when start > end if it is in the range of string (line 147)

Copy link
Contributor Author

@LiiNen LiiNen Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note: you said larger value as 10 or 50 for example, but the value 1000 is already used from others, so I also use 1000 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hwware gently ping, just wondered if you didn't receive above comments alarm.
i hope it didn't bother you.

}

test {BITCOUNT with start, end} {
set s "foobar"
r set s $s
Expand All @@ -150,12 +159,10 @@ start_server {tags {"bitops"}} {
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
Loading