Change BITCOUNT 'end' as optional like BITPOS by LiiNen · Pull Request #118 · valkey-io/valkey

LiiNen · 2024-04-01T15:24:24Z

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]]]

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.

/* 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,

src/commands/bitcount.json

tests/unit/bitops.tcl

LiiNen · 2024-04-02T03:10:29Z

updated it and run COMMAND DOCS for checking it works properly

Note. github commit with 'commit suggestion' won't let me signoff ;-; didnt know that

Signed-off-by: LiiNen <[email protected]>

in case of end is not declared, BYTE|BIT cannot be used, so variable 'isbit' is always 0 Signed-off-by: LiiNen <[email protected]>

Signed-off-by: LiiNen <[email protected]>

Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: LiiNen <[email protected]>

LiiNen · 2024-04-02T13:05:18Z

rebase to avoid spellcheck issue: #139

LiiNen · 2024-04-02T16:45:12Z

seems like issues in testfile. I'll check it

edited)
I have missed that bitcount now accept without end. Removed it in tcl.

assert_error {ERR *syntax*} {r bitcount s 0}

p.s. I have changed it when I made PR to redis, but just missed it in here by human(my) fault 😅

Signed-off-by: LiiNen <[email protected]>

hwware · 2024-04-23T15:25:45Z

tests/unit/bitops.tcl

+        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"]


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

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

Added more testcases; 843c84a

Larger value of start as you said

Larger negative value of start.

Also added large negative value in BITCOUNT with start, end

fix [count_bits ""] -> 0, when start > end if it is in the range of string (line 147)

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 :)

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

LiiNen · 2024-04-23T16:55:17Z

src/commands/bitcount.json

-                                "type": "pure-token",
-                                "token": "BYTE"
+                                "name": "end",
+                                "type": "integer"


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?

hwware · 2024-04-24T18:28:25Z

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

hwware · 2024-04-24T18:32:09Z

But I doubt, maybe for BITPOS and BITCOUNT, the command format:
BITPOS key bit start BYTE | BIT
BITCOUNT key start BYTE | BIT
should be acceptable as well in the future because end is optional

Let us discuss this later

LiiNen · 2024-04-25T01:10:44Z

I agree. If there is no need to consider backward compatibility, then me also want to change: using BYTE|BIT without end.
This might also affect aof, so this have to be discussed later 😢

In case of testcase, I'll work on it asap when i have time :)

Signed-off-by: LiiNen <[email protected]>

codecov · 2024-04-29T14:54:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.19%. Comparing base (e4ead94) to head (153625c).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #118      +/-   ##
============================================
- Coverage     70.22%   70.19%   -0.04%     
============================================
  Files           109      109              
  Lines         59880    59879       -1     
============================================
- Hits          42051    42030      -21     
- Misses        17829    17849      +20

Files	Coverage Δ
src/bitops.c	`94.03% <100.00%> (+0.29%)`	⬆️
src/commands.def	`100.00% <ø> (ø)`

... and 8 files with indirect coverage changes

madolson · 2024-05-27T14:20:04Z

But I doubt, maybe for BITPOS and BITCOUNT, the command format:
BITPOS key bit start BYTE | BIT
BITCOUNT key start BYTE | BIT
should be acceptable as well in the future because end is optional

We can solve this problem in the future.

Discussed in separate meeting, directionally LGTM, just need someone to review the code and merge.

LiiNen · 2024-05-27T14:32:04Z

seems like conflicts came out due to modifying language format (#323)
I'll resolve it in my branch and commit once more

Signed-off-by: LiiNen <[email protected]>

LiiNen · 2024-05-27T15:22:06Z

update done.

while fixing conflict, found small typo against lint, so it also have been fixed.

/* line 798 in src/bitops.c */
// before fix
if (c-> argc == 3 

// after fix ( remove space between '>' and 'a' )
if (c->argc == 3

srgsanky · 2024-05-29T02:53:54Z

There is a clang-format issue from this commit found while trying to submit another commit - https://github.com/valkey-io/valkey/actions/runs/9279101016/job/25531813775. Can you fix it?

diff --git a/src/bitops.c b/src/bitops.c
index 7a385812d..2094bb0ea 100644
--- a/src/bitops.c
+++ b/src/bitops.c
@@ -808,10 +808,9 @@ void bitcountCommand(client *c) {
             }
         }
         if (c->argc >= 4) {
-            if (getLongLongFromObjectOrReply(c,c->argv[3],&end,NULL) != C_OK)
-                return;
+            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;
@@ -821,7 +820,7 @@ void bitcountCommand(client *c) {
         /* Make sure we will not overflow */
         serverAssert(totlen <= LLONG_MAX >> 3);

-        if (c->argc < 4) end = totlen-1;
+        if (c->argc < 4) end = totlen - 1;

         /* Convert negative indexes */
         if (start < 0 && end < 0 && start > end) {

LiiNen · 2024-05-29T03:22:18Z

@srgsanky sorry I dont know why but i missed it.
but this PR already closed (merged) today, so I write a simple change as PR #570
I'll left this also in your PR. thanks that let me know about it.

ref: - #118 (my pervious change) - #461 (issuing that clang format checker fails due to my change) There was an issue that clang-format cheker failed. I don't know why I missed it and why it didn't catch. just running `clang-format -i bitops.c` was all. Signed-off-by: LiiNen <[email protected]>

LiiNen force-pushed the bitcount-end-as-optional branch from 338c5d1 to ecbcc8b Compare April 1, 2024 15:25

madolson added the major-decision-pending Major decision pending by TSC team label Apr 2, 2024

madolson reviewed Apr 2, 2024

View reviewed changes

src/commands/bitcount.json Show resolved Hide resolved

madolson reviewed Apr 2, 2024

View reviewed changes

tests/unit/bitops.tcl Outdated Show resolved Hide resolved

LiiNen force-pushed the bitcount-end-as-optional branch from 3fbebc4 to fd83b40 Compare April 2, 2024 03:08

LiiNen and others added 9 commits April 2, 2024 21:57

Fix to allow end as optional when using bitcount

a6f4296

Signed-off-by: LiiNen <[email protected]>

Fix hint for bitcount

5b067d3

Signed-off-by: LiiNen <[email protected]>

Add test in case for only using start in bitcount

8e813f3

Signed-off-by: LiiNen <[email protected]>

Remove unreachable code in BITCOUNT and BITPOS

64008bb

in case of end is not declared, BYTE|BIT cannot be used, so variable 'isbit' is always 0 Signed-off-by: LiiNen <[email protected]>

Move line end = totlen - 1, after totlen declared

fb2e403

Signed-off-by: LiiNen <[email protected]>

Add commands.def after build

c758112

Signed-off-by: LiiNen <[email protected]>

Add history for this fix

fc369bb

Signed-off-by: LiiNen <[email protected]>

Add commands.def after build

d5f0da6

Signed-off-by: LiiNen <[email protected]>

Update tests/unit/bitops.tcl

729ec8b

Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: LiiNen <[email protected]>

LiiNen force-pushed the bitcount-end-as-optional branch from fd83b40 to 729ec8b Compare April 2, 2024 12:59

Fix test code : allowing bitcount without end now

c0dd4ee

Signed-off-by: LiiNen <[email protected]>

zuiderkwast added the enhancement New feature or request label Apr 2, 2024

hwware reviewed Apr 23, 2024

View reviewed changes

LiiNen commented Apr 23, 2024

View reviewed changes

Add test cases; larger value, negative larger value

843c84a

Signed-off-by: LiiNen <[email protected]>

madolson requested a review from hwware May 27, 2024 14:18

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels May 27, 2024

madolson added the release-notes This issue should get a line item in the release notes label May 27, 2024

Merge branch 'unstable' into bitcount-end-as-optional

153625c

Signed-off-by: LiiNen <[email protected]>

hwware approved these changes May 27, 2024

View reviewed changes

hwware merged commit 96dcd11 into valkey-io:unstable May 28, 2024

srgsanky mentioned this pull request May 29, 2024

Make cluster meet reliable under link failures #461

Merged

LiiNen mentioned this pull request May 29, 2024

Fix bitops.c clang-format properly #570

Merged

asafpamzn mentioned this pull request Jul 28, 2024

Valkey 8 support valkey-io/valkey-glide#2033

Closed

7 tasks

shohamazon mentioned this pull request Aug 19, 2024

Add Valkey8 support valkey-io/valkey-glide#2102

Closed

6 tasks

This was referenced Sep 4, 2024

Node: implement BITPOS change valkey-io/valkey-glide#2225

Closed

Node: fix BITPOS valkey-io/valkey-glide#2227

Merged

Python: bitpos valkey8 valkey-io/valkey-glide#2256

Merged

Conversation

LiiNen commented Apr 1, 2024

Issue

What did I fix

Moreover ...

Uh oh!

Uh oh!

Uh oh!

LiiNen commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiiNen commented Apr 2, 2024

Uh oh!

LiiNen commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwware Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

LiiNen Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

LiiNen Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

LiiNen Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiiNen May 8, 2024

Choose a reason for hiding this comment

Uh oh!

LiiNen Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwware commented Apr 24, 2024

Uh oh!

hwware commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiiNen commented Apr 25, 2024

Uh oh!

codecov bot commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

madolson commented May 27, 2024

Uh oh!

LiiNen commented May 27, 2024

Uh oh!

LiiNen commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srgsanky commented May 29, 2024

Uh oh!

LiiNen commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LiiNen commented Apr 2, 2024 •

edited

Loading

LiiNen commented Apr 2, 2024 •

edited

Loading

LiiNen Apr 29, 2024 •

edited

Loading

LiiNen Apr 23, 2024 •

edited

Loading

hwware commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 29, 2024 •

edited

Loading

LiiNen commented May 27, 2024 •

edited

Loading