feat(tdigest): Implement TDIGEST.CDF command #3163

SharonIV0x86 · 2025-09-06T17:37:29Z

This closes: #2807
This PR implements the support for TDIGEST.CDF command redis docs. Following the implementation of the Quantile function for accessing tdigest centroids.

Will add the unit tests once the current logic of the distribution function is correct and approved.

Copilot

Pull Request Overview

This PR implements the TDIGEST.CDF command to calculate cumulative distribution function values for given inputs in a T-Digest data structure. The implementation follows Redis documentation and adds the necessary command parsing, execution logic, and result handling.

Adds new CDF method to TDigest class that computes cumulative distribution function values
Implements CommandTDigestCDF command class with proper parsing and execution
Registers the new tdigest.cdf command with appropriate attributes

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/types/redis_tdigest.h	Adds TDigestCDFResult struct and CDF method declaration
src/types/redis_tdigest.cc	Implements CDF calculation logic using centroids
src/commands/cmd_tdigest.cc	Adds CommandTDigestCDF class and registers the new command

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-09T06:29:34Z

src/types/redis_tdigest.cc

+    double eq_count = 0;
+    double smaller_count = 0;
+    for (; iter_begin->Valid(); iter_begin->Next()) {
+      auto current_centroid = iter_begin->GetCentroid();
+      if (val > current_centroid->mean) {
+        smaller_count++;
+      } else if (val == current_centroid->mean) {
+        eq_count++;
+      }
+    }
+    double cdf_val = (smaller_count / total_weight) + ((eq_count / 2) / total_weight);


The CDF calculation is incorrect. It's counting the number of centroids rather than their weights. It should accumulate current_centroid->weight instead of incrementing by 1 for proper cumulative distribution calculation.

Suggested change

double eq_count = 0;

double smaller_count = 0;

for (; iter_begin->Valid(); iter_begin->Next()) {

auto current_centroid = iter_begin->GetCentroid();

if (val > current_centroid->mean) {

smaller_count++;

} else if (val == current_centroid->mean) {

eq_count++;

}

}

double cdf_val = (smaller_count / total_weight) + ((eq_count / 2) / total_weight);

double eq_weight = 0;

double smaller_weight = 0;

for (; iter_begin->Valid(); iter_begin->Next()) {

auto current_centroid = iter_begin->GetCentroid();

if (val > current_centroid->mean) {

smaller_weight += current_centroid->weight;

} else if (val == current_centroid->mean) {

eq_weight += current_centroid->weight;

}

}

double cdf_val = (smaller_weight / total_weight) + ((eq_weight / 2) / total_weight);

Hi @SharonIV0x86 ,

It seems that we mistake the count with weight here.

Okay, I will take a look into it.

Copilot · 2025-09-09T06:29:35Z

src/commands/cmd_tdigest.cc

                        MakeCmdAttr<CommandTDigestReset>("tdigest.reset", 2, "write", 1, 1, 1),
-                        MakeCmdAttr<CommandTDigestMerge>("tdigest.merge", -4, "write", GetMergeKeyRange));
+                        MakeCmdAttr<CommandTDigestMerge>("tdigest.merge", -4, "write", GetMergeKeyRange),
+                        MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "write", 1, 1, 1));


The TDIGEST.CDF command should be marked as 'read-only' instead of 'write' since it only reads data and doesn't modify the T-Digest structure.

Suggested change

MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "write", 1, 1, 1));

MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "read-only", 1, 1, 1));

Copilot · 2025-09-09T06:29:35Z

src/commands/cmd_tdigest.cc

+class CommandTDigestCDF : public Commander {
+  Status Parse(const std::vector<std::string> &args) override {
+    key_name_ = args[1];
+    if (args.size() == 2) return {Status::RedisParseErr, errWrongNumOfArguments};


The command registration specifies minimum 4 arguments (-4), but this validation only checks for exactly 2 arguments. It should validate that there are at least 3 arguments (command + key + at least one value).

Suggested change

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

if (args.size() < 3) return {Status::RedisParseErr, errWrongNumOfArguments};

LindaSummer · 2025-09-09T06:30:10Z

Hi @SharonIV0x86 ,

Thanks for your effort!
I will review this PR later today. 😊

Best Regards,
Edward

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your effort! 😊

Left some comments and we needto add unit tests and integration tests for this command.

Best Regards,
Edward

LindaSummer · 2025-09-09T14:59:08Z

src/commands/cmd_tdigest.cc

+class CommandTDigestCDF : public Commander {
+  Status Parse(const std::vector<std::string> &args) override {
+    key_name_ = args[1];
+    if (args.size() == 2) return {Status::RedisParseErr, errWrongNumOfArguments};


Maybe we could check the vector size at beginning.

LindaSummer · 2025-09-09T15:07:12Z

src/commands/cmd_tdigest.cc

+      }
+      return {Status::RedisExecErr, meta_status.ToString()};
+    }
+    if (metadata.total_observations == 0) {


I have tested with Redis Docker, it should be the ["nan"] vector with the same size as the input.

LindaSummer · 2025-09-09T15:08:42Z

src/commands/cmd_tdigest.cc

+      return {Status::RedisExecErr, s.ToString()};
+    }
+    for (const auto &val : result.cdf_values) {
+      cdf_result.push_back(std::to_string(val));


We could use util::Float2String as the quantile command.

LindaSummer · 2025-09-09T15:09:38Z

src/commands/cmd_tdigest.cc

                        MakeCmdAttr<CommandTDigestReset>("tdigest.reset", 2, "write", 1, 1, 1),
-                        MakeCmdAttr<CommandTDigestMerge>("tdigest.merge", -4, "write", GetMergeKeyRange));
+                        MakeCmdAttr<CommandTDigestMerge>("tdigest.merge", -4, "write", GetMergeKeyRange),
+                        MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "write", 1, 1, 1));


It is a read-only command, but with a lock guard in merge action.

Yes, i will work on it, i was unsure about how the command registration works and what exactly those parameters do in the MakeCmdAttr so it will be helpful if you could explain it in a little detail.
Thanks.

You can refer to PR #2620 for further understanding of the MakeCmdAttr flags.

kvrocks/src/server/redis_connection.cc

Lines 528 to 546 in cf28ab1

SetLastCmd(cmd_name);

{

std::optional<MultiLockGuard> guard;

if (cmd_flags & kCmdWrite) {

std::vector<std::string> lock_keys;

attributes->ForEachKeyRange(

[&lock_keys, this](const std::vector<std::string> &args, const CommandKeyRange &key_range) {

key_range.ForEachKey(

[&, this](const std::string &key) {

auto ns_key = ComposeNamespaceKey(ns_, key, srv_->storage->IsSlotIdEncoded());

lock_keys.emplace_back(std::move(ns_key));

},

args);

},

cmd_tokens);

guard.emplace(srv_->storage->GetLockManager(), lock_keys);

}

engine::Context ctx(srv_->storage);

I have made this change.

sonarqubecloud · 2025-09-12T11:35:10Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 50%)
37.7% Duplication on New Code (required ≤ 20%)

See analysis details on SonarQube Cloud

…451ba

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-06T15:17:34Z

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

+		require.Len(t, vals, 1)
+		require.Equal(t, "nan", vals[0])
+
+		// testing with a empry digest with multi-valued CDF


Corrected spelling of 'empry' to 'empty'.

Suggested change

// testing with a empry digest with multi-valued CDF

// testing with an empty digest with multi-valued CDF

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your effort! 😊

Leave comments, and we'd better also check the comments from Copilot,

Best Regards,
Edward

LindaSummer · 2025-10-06T15:22:41Z

src/commands/cmd_tdigest.cc

+    TDigestCDFResult result;
+    TDigestMetadata metadata;
+    auto meta_status = tdigest.GetMetaData(ctx, key_name_, &metadata);
+    std::vector<std::string> nan_results(values_.size(), "nan");


The nan_results could be constructed in the empty element branch.

LindaSummer · 2025-10-06T15:30:27Z

src/commands/cmd_tdigest.cc

+    TDigestCDFResult result;
+    TDigestMetadata metadata;
+    auto meta_status = tdigest.GetMetaData(ctx, key_name_, &metadata);
+    std::vector<std::string> nan_results(values_.size(), "nan");


Hi @SharonIV0x86 ,

We could move this nan_results construction into the empty element branch.

LindaSummer · 2025-10-06T15:30:31Z

src/types/redis_tdigest.cc

+    double eq_count = 0;
+    double smaller_count = 0;
+    for (; iter_begin->Valid(); iter_begin->Next()) {
+      auto current_centroid = iter_begin->GetCentroid();
+      if (val > current_centroid->mean) {
+        smaller_count++;
+      } else if (val == current_centroid->mean) {
+        eq_count++;
+      }
+    }
+    double cdf_val = (smaller_count / total_weight) + ((eq_count / 2) / total_weight);


Hi @SharonIV0x86 ,

It seems that we mistake the count with weight here.

LindaSummer · 2025-10-06T15:32:41Z

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

+		// create a tdigest and add some data
+		tdigestKey := keyPrefix + "source"
+		require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", tdigestKey).Err())
+		require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", tdigestKey, "1.0", "2.0", "3.0", "4.0", "5.0").Err())


Hi @SharonIV0x86 ,

We'd better add some tests with duplicated values to create different weights for some centroids.

SharonIV0x86 and others added 3 commits September 6, 2025 22:17

feat: added initial tdigest.cdf logic

3235803

Merge branch 'apache:unstable' into feat/tdigest.cdf-command

f3bdd77

fix: fixed typo in inputs and registered tdigest.cdf in MakeCmdAttr

9e26d86

SharonIV0x86 marked this pull request as ready for review September 8, 2025 03:36

LindaSummer requested review from LindaSummer and Copilot September 9, 2025 06:28

Copilot AI reviewed Sep 9, 2025

View reviewed changes

LindaSummer reviewed Sep 9, 2025

View reviewed changes

SharonIV0x86 and others added 4 commits September 9, 2025 22:28

Merge branch 'apache:unstable' into feat/tdigest.cdf-command

7366edb

Merge branch 'apache:unstable' into feat/tdigest.cdf-command

d3a44cf

fix: fixed MakeCmdAttr to read-only

262574f

fix: used util::float2String instead of to_string.

b64389d

Merge branch 'unstable' into feat/tdigest.cdf-command

4731484

SharonIV0x86 marked this pull request as draft September 14, 2025 17:13

SharonIV0x86 and others added 11 commits September 14, 2025 23:59

Added gocase unit test case.

f361fc4

Merge branch 'unstable' into feat/tdigest.cdf-command

6753870

progress: added basic c++ unit test case for CDF

cb86cb3

Merge branch 'unstable' into feat/tdigest.cdf-command

fcd7d5a

progress: added more c++ test cases for cdf

e0451ba

fix: fixed go test file formatting, distorted from the prev commit e0…

1d425a0

…451ba

Merge branch 'unstable' into feat/tdigest.cdf-command

31423cd

fix clang-tidy error

67923f4

Merge branch 'unstable' into feat/tdigest.cdf-command

4d1988b

Merge branch 'unstable' into feat/tdigest.cdf-command

ec52575

fix: go formatting

d23ad76

SharonIV0x86 marked this pull request as ready for review October 5, 2025 13:22

LindaSummer requested a review from Copilot October 6, 2025 15:17

Copilot AI reviewed Oct 6, 2025

View reviewed changes

LindaSummer reviewed Oct 6, 2025

View reviewed changes

SharonIV0x86 marked this pull request as draft October 9, 2025 10:46

Merge branch 'unstable' into feat/tdigest.cdf-command

cbff56e

	MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "write", 1, 1, 1));
	MakeCmdAttr<CommandTDigestCDF>("tdigest.cdf", -4, "read-only", 1, 1, 1));

	if (args.size() == 2) return {Status::RedisParseErr, errWrongNumOfArguments};
	if (args.size() < 3) return {Status::RedisParseErr, errWrongNumOfArguments};

	SetLastCmd(cmd_name);
	{
	std::optional<MultiLockGuard> guard;
	if (cmd_flags & kCmdWrite) {
	std::vector<std::string> lock_keys;
	attributes->ForEachKeyRange(
	[&lock_keys, this](const std::vector<std::string> &args, const CommandKeyRange &key_range) {
	key_range.ForEachKey(
	[&, this](const std::string &key) {
	auto ns_key = ComposeNamespaceKey(ns_, key, srv_->storage->IsSlotIdEncoded());
	lock_keys.emplace_back(std::move(ns_key));
	},
	args);
	},
	cmd_tokens);

	guard.emplace(srv_->storage->GetLockManager(), lock_keys);
	}
	engine::Context ctx(srv_->storage);

	// testing with a empry digest with multi-valued CDF
	// testing with an empty digest with multi-valued CDF

feat(tdigest): Implement TDIGEST.CDF command #3163

Are you sure you want to change the base?

feat(tdigest): Implement TDIGEST.CDF command #3163

Uh oh!

Conversation

SharonIV0x86 commented Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 9, 2025

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!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer commented Sep 9, 2025

Uh oh!

LindaSummer left a comment

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!

LindaSummer Sep 10, 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!

SharonIV0x86 Sep 11, 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!

sonarqubecloud bot commented Sep 12, 2025

Quality Gate failed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SharonIV0x86 commented Sep 6, 2025 •

edited

Loading

LindaSummer Sep 10, 2025 •

edited

Loading

SharonIV0x86 Sep 11, 2025 •

edited

Loading