feat(digest): implement DIGEST command #3313

chakkk309 · 2025-12-26T15:58:41Z

src/commands/cmd_string.cc

tests/gocase/unit/type/string/digest_test.go

git-hulk · 2025-12-27T14:11:43Z

tests/gocase/unit/type/string/digest_test.go

+		require.NoError(t, rdb.Set(ctx, "key1", "Hello world", 0).Err())
+
+		// DIGEST should return hex hash
+		digest := rdb.Do(ctx, "DIGEST", "key1").Val()


Can use the String to avoid the typecast. The same as other places.

Suggested change

digest := rdb.Do(ctx, "DIGEST", "key1").Val()

digest := rdb.Do(ctx, "DIGEST", "key1").String()

Can use the String to avoid the typecast. The same as other places.

Thanks for the suggestion, I will update other places.

Co-authored-by: hulk <hulk.website@gmail.com>

git-hulk · 2025-12-27T14:22:52Z

@chakkk309 The CI failure is due to the missing ASF license header in the new file; you could also fix it. https://github.com/apache/kvrocks/actions/runs/20540153574/job/59003405075?pr=3313#step:4:139

…o feat-implement-DIGEST-command

chakkk309 · 2025-12-27T14:27:55Z

@chakkk309 The CI failure is due to the missing ASF license header in the new file; you could also fix it. https://github.com/apache/kvrocks/actions/runs/20540153574/job/59003405075?pr=3313#step:4:139

Thanks for the reminder, I have already added the license header~

git-hulk

Generally looks good to me, one test case suggestion inline.

@chakkk309, Could you please remove the meaningless comments inside test cases?

tests/gocase/unit/type/string/digest_test.go

Co-authored-by: hulk <hulk.website@gmail.com>

…o feat-implement-DIGEST-command

Valay17 · 2025-12-28T04:01:13Z

I was going through your commits and observed that you have implemented the digest function directly in the cmd_string.cc itself, making it noncallable as other commands depend on it and making it difficult to adapt changes further down the line. Type checking for key is not implemented as well, key must be of type string for it to be hashed

git-hulk · 2025-12-28T12:54:14Z

@chakkk309 XXH3_64bits haven't been implemented in Kvrocks.

PragmaTwice · 2025-12-28T13:48:42Z

src/types/redis_string.cc

+std::string String::ComputeXXH3Hash(const std::string &data) {
+  uint64_t hash = XXH3_64bits(data.data(), data.size());
+  return fmt::format("{:016x}", hash);
+}


This method seems unnecessary.

Actually no, because the DELEX and SET command relies on this command so keeping a method is the right way to do it. If we do it this way it will be easier to add more commands in the future that requires DIGEST and also change the hashing algorithm by just changing this 1 function instead of changing it multiple times in various places.

I'm fine with this utility. But it should not be a method of redis::String since it does nothing related to rocksdb IO.

It can be put into string_util.h.

It can be put into string_util.h.

Sure, i will move this method later.

PragmaTwice · 2025-12-28T13:56:01Z

src/commands/cmd_string.cc

+      Config *config = srv->GetConfig();
+      uint32_t max_btos_size = static_cast<uint32_t>(config->max_bitmap_to_string_mb) * MiB;
+      redis::Bitmap bitmap_db(srv->storage, conn->GetNamespace());
+      std::string value;
+      s = bitmap_db.GetString(ctx, args_[1], max_btos_size, &value);
+      if (s.ok()) {
+        digest = redis::String::ComputeXXH3Hash(value);
+      }


Should we put them in redis_bitmap.cc? @git-hulk

PragmaTwice · 2025-12-28T13:57:08Z

src/types/redis_string.cc

+rocksdb::Status String::Digest(engine::Context &ctx, const std::string &user_key, std::string *digest) {
+  std::string value;
+  auto s = Get(ctx, user_key, &value);
+  if (!s.ok()) {
+    return s;
+  }
+
+  *digest = ComputeXXH3Hash(value);
+  return rocksdb::Status::OK();
+}
+


I'm not sure if we can unify the bitmap and string part into one place. Or maybe we can just drop the support of bitmap? @git-hulk

I totally agree to drop the support of bitmap in string, because it might introduce the unexpected performance issue if users incorrectly read the bitmap as a string.

PragmaTwice · 2025-12-29T02:16:35Z

src/commands/cmd_string.cc

 #include <string>
-
-#include "commander.h"
+#include <fmt/format.h>


remove this pls.

PragmaTwice · 2025-12-29T02:16:54Z

src/commands/cmd_string.cc

+    if (s.IsInvalidArgument()) {
+      Config *config = srv->GetConfig();
+      uint32_t max_btos_size = static_cast<uint32_t>(config->max_bitmap_to_string_mb) * MiB;
+      redis::Bitmap bitmap_db(srv->storage, conn->GetNamespace());
+      std::string value;
+      s = bitmap_db.GetString(ctx, args_[1], max_btos_size, &value);
+      if (s.ok()) {
+        digest = util::ComputeXXH3Hash(value);
+      }
+    }


Thanks, i will remove bitmap related code.

PragmaTwice · 2025-12-29T02:17:03Z

src/commands/cmd_string.cc

-#include "commander.h"
+#include <fmt/format.h>
 #include "commands/command_parser.h"
+#include "common/string_util.h"


src/common/string_util.cc

PragmaTwice · 2025-12-29T02:18:41Z

tests/gocase/unit/type/string/digest_test.go

+		{"simple string", "hello", ""},
+		{"number as string", "123", ""},
+		{"special chars", "!@#$%^&*()", ""},
+		{"unicode", "こんにちは", ""},


please fill these expcted value..

Thanks for the reminder, i 've already added.

PragmaTwice

Please read https://kvrocks.apache.org/community/contributing#guidelines-for-ai-assisted-contributions.

Co-authored-by: Twice <twice@apache.org>

chakkk309 · 2025-12-29T11:22:22Z

Please read https://kvrocks.apache.org/community/contributing#guidelines-for-ai-assisted-contributions.

Thanks for sharing. I've read the AI contribution guidelines and will follow them, ensuring code quality through manual review and testing.

src/types/redis_string.cc

PragmaTwice · 2025-12-29T12:17:42Z

CI workflow failed: https://github.com/apache/kvrocks/actions/runs/20572608610/job/59082847157?pr=3313.

Please format your code before pushing them.

git-hulk · 2025-12-30T05:34:28Z

@chakkk309 You could try to build and test this PR on your side first.

PragmaTwice · 2025-12-30T05:46:18Z

@chakkk309 Hey, please read our AI contribution guide again. Please make sure you can understand the AI-generated code and are able to build and test your changes yourself. This PR has been in a state for a while where it still can’t build successfully or pass tests, and if that continues, I will stop reviewing and close this PR.

chakkk309 · 2025-12-30T11:49:05Z

@chakkk309 Hey, please read our AI contribution guide again. Please make sure you can understand the AI-generated code and are able to build and test your changes yourself. This PR has been in a state for a while where it still can’t build successfully or pass tests, and if that continues, I will stop reviewing and close this PR.

Thanks you, I ran ./x.py check golangci-lint locally to make sure I’ve fixed all the lint errors.

chakkk309 · 2025-12-30T11:51:38Z

@chakkk309 You could try to build and test this PR on your side first.

Thanks, I’ve built it successfully on my local machine and also tested to make sure I fixed the lint error.

sonarqubecloud · 2025-12-30T16:00:03Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
76.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

PragmaTwice · 2025-12-30T16:25:21Z

tests/gocase/unit/type/digest/digest_test.go

digest is not a "type". please move this file to unit.

feat: implement DIGEST command

fdad4ab

git-hulk reviewed Dec 27, 2025

View reviewed changes

git-hulk requested a review from PragmaTwice December 27, 2025 14:13

chakkk309 and others added 2 commits December 27, 2025 22:16

Update src/commands/cmd_string.cc

e03b477

Co-authored-by: hulk <hulk.website@gmail.com>

Update tests/gocase/unit/type/string/digest_test.go

624ce78

Co-authored-by: hulk <hulk.website@gmail.com>

chakkk309 added 3 commits December 27, 2025 22:24

style: use String() method instead of typecast in digest tests

db10ddd

Merge remote-tracking branch 'fork/feat-implement-DIGEST-command' int…

ec7052e

…o feat-implement-DIGEST-command

style: add license header in digest test

b9e603c

chakkk309 requested a review from git-hulk December 27, 2025 14:28

git-hulk reviewed Dec 27, 2025

View reviewed changes

tests/gocase/unit/type/string/digest_test.go Outdated Show resolved Hide resolved

chakkk309 and others added 4 commits December 27, 2025 22:55

style: fix clang-format issues

302b8d0

Update tests/gocase/unit/type/string/digest_test.go

02670d7

Co-authored-by: hulk <hulk.website@gmail.com>

Merge remote-tracking branch 'fork/feat-implement-DIGEST-command' int…

928dcf6

…o feat-implement-DIGEST-command

style: remove meaningless comments from digest test

20974ca

refactor: implement DIGEST command

694b53a

PragmaTwice reviewed Dec 28, 2025

View reviewed changes

chakkk309 added 3 commits December 28, 2025 23:18

fix: revert DIGEST tests to use Val() method instead of String()

d1120f8

fix: clang format error

2226341

refactor: move ComputeXXH3Hash from redis::String to util

8623728

PragmaTwice reviewed Dec 29, 2025

View reviewed changes

src/common/string_util.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed Dec 29, 2025

View reviewed changes

src/common/string_util.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed Dec 29, 2025

View reviewed changes

chakkk309 and others added 4 commits December 29, 2025 18:10

Update src/common/string_util.cc

c9e1744

Co-authored-by: Twice <twice@apache.org>

Update src/common/string_util.cc

0c93d42

Co-authored-by: Twice <twice@apache.org>

fix: remove bitmap support from DIGEST command

0c3f318

fix: digest test expected hash values

d5da219

PragmaTwice reviewed Dec 29, 2025

View reviewed changes

src/types/redis_string.cc Outdated Show resolved Hide resolved

Update src/types/redis_string.cc

d2618f3

chakkk309 added 3 commits December 29, 2025 22:08

style: fix clang format

c7a99dd

fix: remove useless import in digest test

ca1dbdb

fix: update special char hash except value in digest test

4aaddd3

style: fix golang lint

0686f26

Merge branch 'unstable' into feat-implement-DIGEST-command

be507dd

PragmaTwice reviewed Dec 30, 2025

View reviewed changes

tests/gocase/unit/type/digest/digest_test.go

Copy link

Member

PragmaTwice Dec 30, 2025

Choose a reason for hiding this comment

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

digest is not a "type". please move this file to unit.

	digest := rdb.Do(ctx, "DIGEST", "key1").Val()
	digest := rdb.Do(ctx, "DIGEST", "key1").String()

feat(digest): implement DIGEST command #3313

Are you sure you want to change the base?

feat(digest): implement DIGEST command #3313

Conversation

chakkk309 commented Dec 26, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

git-hulk commented Dec 27, 2025

Uh oh!

chakkk309 commented Dec 27, 2025

Uh oh!

git-hulk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Valay17 commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

git-hulk commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragmaTwice left a comment

Choose a reason for hiding this comment

Uh oh!

chakkk309 commented Dec 29, 2025

Uh oh!

Uh oh!

PragmaTwice commented Dec 29, 2025

Uh oh!

git-hulk commented Dec 30, 2025

Uh oh!

PragmaTwice commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chakkk309 commented Dec 30, 2025

Uh oh!

chakkk309 commented Dec 30, 2025

Uh oh!

sonarqubecloud bot commented Dec 30, 2025

Quality Gate passed

Uh oh!

Valay17 commented Dec 28, 2025 •

edited

Loading

git-hulk commented Dec 28, 2025 •

edited

Loading

PragmaTwice commented Dec 30, 2025 •

edited

Loading