feat(tdigest): add TDIGEST.CREATE and TDIGEST.INFO command by LindaSummer · Pull Request #2799 · apache/kvrocks

LindaSummer · 2025-02-21T15:43:07Z

Issue

Close #2791

Proposed Changes

implement tdigest.add command.
implement tdigest.info command.
add integration tests.

Comment

redis tdigest.info has one property Memory usage, and kvrocks's data structure is not in memory, could you give me some suggestions on evaluating the memory usage? 😊
in refactor: hoist key mutexes to ExecuteCommands #2620 , we refactor the lock to outside of the command.
So, I will remove locks after adding "write" property to commands.
For quantile command I plan to use "read" property and manually manage the lock to improve performance.

PragmaTwice · 2025-02-21T15:50:34Z

src/commands/cmd_tdigest.cc

+    if (args.size() != 2) {
+      return {Status::RedisParseErr, kErrWrongNumOfInfoArguments};
+    }


Suggested change

if (args.size() != 2) {

return {Status::RedisParseErr, kErrWrongNumOfInfoArguments};

}

This check is redundant.

Hi @PragmaTwice ,

Got it! I have removed the redundant code. 😊

Best Regards,
Edward

PragmaTwice · 2025-02-21T15:52:07Z

src/commands/cmd_tdigest.cc

+    output->append(redis::BulkString(kInfoMemoryUsage));
+    output->append(redis::Integer(sizeof(TDigest)));


It seems this is not so useful, and also, not so accurate. Maybe we can just remove this.

Hi @PragmaTwice ,

Thanks for your suggestion! 😊

I have removed this property and left a comment line in code and test case.

Best Regards,
Edward

PragmaTwice · 2025-02-21T15:59:26Z

src/commands/cmd_tdigest.cc

+    bool invalid_keyword = false;
+    if (args.size() != 2 && (args.size() != 4 || (invalid_keyword = !util::EqualICase(kCompressionArg, args[2])))) {
+      return {Status::RedisParseErr, invalid_keyword ? kErrWrongKeyword : kErrWrongNumOfCreateArguments};
+    }
+    if (parser.EatEqICase(kCompressionArg)) {
+      auto status = parser.TakeInt<int32_t>();
+      if (!status) {
+        return {Status::RedisParseErr, kErrParseCompression};
+      }
+      auto compression = *status;
+      if (compression <= 0) {
+        return {Status::RedisParseErr, kErrCompressionMustBePositive};
+      }
+      if (compression < 1 || compression > static_cast<int32_t>(kTDigestMaxCompression)) {
+        return {Status::RedisParseErr, kErrCompressionOutOfRange};
+      }
+      options_.compression = static_cast<uint32_t>(compression);
+    }


Suggested change

bool invalid_keyword = false;

if (args.size() != 2 && (args.size() != 4 || (invalid_keyword = !util::EqualICase(kCompressionArg, args[2])))) {

return {Status::RedisParseErr, invalid_keyword ? kErrWrongKeyword : kErrWrongNumOfCreateArguments};

}

if (parser.EatEqICase(kCompressionArg)) {

auto status = parser.TakeInt<int32_t>();

if (!status) {

return {Status::RedisParseErr, kErrParseCompression};

}

auto compression = *status;

if (compression <= 0) {

return {Status::RedisParseErr, kErrCompressionMustBePositive};

}

if (compression < 1 || compression > static_cast<int32_t>(kTDigestMaxCompression)) {

return {Status::RedisParseErr, kErrCompressionOutOfRange};

}

options_.compression = static_cast<uint32_t>(compression);

}

if (parser.EatEqICase(kCompressionArg)) {

auto status = parser.TakeInt<int32_t>();

if (!status) {

return {Status::RedisParseErr, kErrParseCompression};

}

auto compression = *status;

if (compression <= 0) {

return {Status::RedisParseErr, kErrCompressionMustBePositive};

}

if (compression < 1 || compression > static_cast<int32_t>(kTDigestMaxCompression)) {

return {Status::RedisParseErr, kErrCompressionOutOfRange};

}

options_.compression = static_cast<uint32_t>(compression);

}

if (parser.Good()) {

return ... // invalid arguments

}

Hi @PragmaTwice ,

Thanks for your advice! 😊

I have refactored the block with parser.Good().

Best Regards,
Edward

git-hulk · 2025-02-22T02:49:51Z

src/commands/cmd_tdigest.cc

+namespace redis {
+namespace {
+constexpr auto kCompressionArg = "compression";
+constexpr auto kErrWrongKeyword = "T-Digest: wrong keyword";


We could remove the prefix(T-Digest) in the error message to be consistent with other commands. Users should know the context according to the command they sent, so that we can avoid creating a dedicated error message for each command.

Hi @git-hulk ,

Got it! Thanks for your suggestions! 😊

I have removed all T-Digest: prefixes from error message.

Best Regards,
Edward

git-hulk · 2025-02-22T03:57:10Z

src/commands/cmd_tdigest.cc

+
+namespace redis {
+namespace {
+constexpr auto kCompressionArg = "compression";


Could put those error messages into https://github.com/apache/kvrocks/blob/9d2fcdef3bdff9f01451ae72491b3a438b9e6a2f/src/commands/error_constants.h, sorry for forgetting to mention this in the last review. Others are good to me, thanks for your contribution.

Hi @git-hulk ,

Of course! 😊
I have moved error messages to error_constants.h.

Best Regards,
Edward

PragmaTwice · 2025-02-22T04:07:33Z

src/commands/cmd_tdigest.cc

+      parser.RawTake();
+      return {Status::RedisParseErr, parser.Good() ? kErrWrongKeyword : kErrWrongNumOfArguments};


Suggested change

parser.RawTake();

return {Status::RedisParseErr, parser.Good() ? kErrWrongKeyword : kErrWrongNumOfArguments};

return {Status::RedisParseErr, kErrWrongNumOfArguments};

Hi @PragmaTwice ,

Thanks for your suggestion! 😊

I have simplified the logic as suggested.

Best Regards,
Edward

PragmaTwice · 2025-02-22T04:09:29Z

tests/gocase/unit/type/tdigest/tdigest_test.go

+		// require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", keyPrefix+"key1", "compression", "1000").Err())
+		// rsp = rdb.Do(ctx, "TDIGEST.INFO", keyPrefix+"key1")
+		// require.NoError(t, rsp.Err())
+		// info = toTdigestInfo(t, rsp.Val())
+		// require.EqualValues(t, 1000, info.Compression)
+		// require.EqualValues(t, 1024, info.Capacity) // max is 1024
+		// require.EqualValues(t, 0, info.MergedNodes)
+		// require.EqualValues(t, 0, info.UnmergedNodes)
+		// require.EqualValues(t, 0, info.MergedWeight)
+		// require.EqualValues(t, 0, info.UnmergedWeight)
+		// require.EqualValues(t, 0, info.Observations)
+		// require.EqualValues(t, 0, info.TotalCompressions)


Hmm what's the situation of this commented block?

Hmm what's the situation of this commented block?

Oh, my mistake! Sorry that I think I forgot to recover some parts for debug. I will double check my commit today.

Hi @PragmaTwice ,

Thanks very much for your correction! ❤

I have recovered the commented test case and have run it in my local environment.

Best Regards,
Edward

src/commands/cmd_tdigest.cc

src/types/redis_tdigest.h

src/commands/cmd_tdigest.cc

sonarqubecloud · 2025-02-22T13:43:56Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

LindaSummer added 3 commits February 21, 2025 22:05

add create and info commands for tdigest.

fb6ea01

fix create and info command.

78b14e5

Add integration tests.

62af97f

PragmaTwice changed the title ~~feat(tdigest): add create and info command.~~ feat(tdigest): add create and info command.\ Feb 21, 2025

PragmaTwice changed the title ~~feat(tdigest): add create and info command.\~~ feat(tdigest): add create and info command Feb 21, 2025

PragmaTwice reviewed Feb 21, 2025

View reviewed changes

git-hulk reviewed Feb 22, 2025

View reviewed changes

LindaSummer added 3 commits February 22, 2025 11:27

Refactor code for comments.

44c5d49

Merge branch 'unstable' into feature/tdigest-add-and-info

d8eed28

Fix typo.

3622c1d

LindaSummer force-pushed the feature/tdigest-add-and-info branch from 8b92ed4 to 3622c1d Compare February 22, 2025 03:52

git-hulk reviewed Feb 22, 2025

View reviewed changes

PragmaTwice reviewed Feb 22, 2025

View reviewed changes

git-hulk and others added 2 commits February 22, 2025 16:33

Merge branch 'unstable' into feature/tdigest-add-and-info

515715b

Refactor error messages and fix for comments.

1d0c401

PragmaTwice reviewed Feb 22, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

Update src/commands/cmd_tdigest.cc

5c2454b

PragmaTwice reviewed Feb 22, 2025

View reviewed changes

src/types/redis_tdigest.h Outdated Show resolved Hide resolved

Update src/types/redis_tdigest.h

d0a1616

PragmaTwice reviewed Feb 22, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

Update src/commands/cmd_tdigest.cc

d16af11

PragmaTwice approved these changes Feb 22, 2025

View reviewed changes

PragmaTwice changed the title ~~feat(tdigest): add create and info command~~ feat(tdigest): add TDIGEST.CREATE and TDIGEST.INFO command Feb 22, 2025

PragmaTwice merged commit 64fbd28 into apache:unstable Feb 22, 2025
35 checks passed

LindaSummer mentioned this pull request Feb 26, 2025

TDigest: Implement QUANTILE command for TDigest Algorithm #2794

Closed

2 tasks

	if (args.size() != 2) {
	return {Status::RedisParseErr, kErrWrongNumOfInfoArguments};
	}

		output->append(redis::BulkString(kInfoMemoryUsage));
		output->append(redis::Integer(sizeof(TDigest)));

		parser.RawTake();
		return {Status::RedisParseErr, parser.Good() ? kErrWrongKeyword : kErrWrongNumOfArguments};

Conversation

LindaSummer commented Feb 21, 2025

Issue

Proposed Changes

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!

git-hulk Feb 22, 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!

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!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 22, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

git-hulk Feb 22, 2025 •

edited

Loading