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 <[email protected]>

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 <[email protected]>

…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);
+      }
+    }


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"


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

src/common/string_util.cc

  return s;
 }

+std::string ComputeXXH3Hash(const std::string &data) {


Suggested change

std::string ComputeXXH3Hash(const std::string &data) {

std::string StringDigest(std::string_view data) {

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

src/common/string_util.cc

 }

+std::string ComputeXXH3Hash(const std::string &data) {
+  uint64_t hash = XXH3_64bits(data.data(), data.size());


Suggested change

uint64_t hash = XXH3_64bits(data.data(), data.size());

XXH64_hash_t hash = XXH3_64bits(data.data(), data.size());

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

PragmaTwice

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

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 <[email protected]>

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

624ce78

Co-authored-by: hulk <[email protected]>

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 <[email protected]>

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/commands/cmd_string.cc

#include <string>

#include "commander.h"

#include <fmt/format.h>

Copy link

Member

PragmaTwice Dec 29, 2025

Choose a reason for hiding this comment

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

remove this pls.

PragmaTwice reviewed Dec 29, 2025

View reviewed changes

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

	std::string ComputeXXH3Hash(const std::string &data) {
	std::string StringDigest(std::string_view data) {

	uint64_t hash = XXH3_64bits(data.data(), data.size());
	XXH64_hash_t hash = XXH3_64bits(data.data(), data.size());

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!

PragmaTwice Dec 29, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Valay17 commented Dec 28, 2025 •

edited

Loading

git-hulk commented Dec 28, 2025 •

edited

Loading

PragmaTwice Dec 29, 2025 •

edited

Loading