feat(tdigest): Add support for QUANTILE command #2849

SharonIV0x86 · 2025-03-23T07:19:43Z

This PR adds support for TDIGEST.QUANTILE command.

SharonIV0x86 · 2025-03-30T18:25:43Z

@LindaSummer I have added the basic structure for the QUANTILE command but as you mentioned in the issue, there seems to be a problem with the locking mechanism or something else.

E20250330 23:50:42.491079 130596879718080 redis_tdigest.cc:524] metadata has 24 merged nodes, but got 4

I tried modifying the quantile function as mentioned in the issue and the previous PRs but the issue persists. Any help is appreciated.

LindaSummer · 2025-04-01T03:42:09Z

Hi @SharonIV0x86 ,

Thanks very much for effort!😊

I'll try to test it in local in recent days and will sync if there is any new updates.

Best Regards,
Edward

SharonIV0x86 · 2025-04-01T10:26:50Z

Hi @SharonIV0x86 ,

Thanks very much for effort!😊

I'll try to test it in local in recent days and will sync if there is any new updates.

Best Regards, Edward

Works, till then ill try to fiddle around. Cheers.

LindaSummer · 2025-04-08T14:12:07Z

Hi @SharonIV0x86 ,

Sorry for delay of updating. 😊

In recent I have some personal affairs which occupied my schedule so maybe a little delay in response.

I'm now working on this ticket and find that there may be some bugs inside it. I will double confirm it and create a PR to fix it if bug exists.

Best Regards,
Edward

SharonIV0x86 · 2025-04-08T14:39:28Z

Hi @SharonIV0x86 ,

Sorry for delay of updating. 😊

In recent I have some personal affairs which occupied my schedule so maybe a little delay in response.

I'm now working on this ticket and find that there may be some bugs inside it. I will double confirm it and create a PR to fix it if bug exists.

Best Regards, Edward

No issues, meanwhile i tried looking into it, no matter what i did there seems to be a mismatch between the actual information and the tdigest metadata now one thing i can deduce is that is not in the Quantile function but this seems to be the issue with mergeCurrentBuffer function or some other centroids function being called from the Quantile function.

Or this is related to locks as you suggested.

There is no hurry from my side in fixing this issue but i am also planning to implement the TDIGEST.CDF and TDIGEST.RANK in upcoming weeks and they most likely will depend in this issue.

LindaSummer · 2025-04-13T13:01:04Z

Hi @SharonIV0x86 ,

I have tested the command line successfully with removing the lock and announcing the command "write" for quick validation after fixing #2878 .

I will try to find a way to improve the performance with less critical section.

Best Regards,
Edward

SharonIV0x86 · 2025-04-13T13:16:57Z

Hi @SharonIV0x86 ,

I have tested the command line successfully with removing the lock and announcing the command "write" for quick validation after fixing #2878 .

I will try to find a way to improve the performance with less critical section.

Best Regards, Edward

Okay, thanks for looking into it. So, how should i proceed then? should i wait for #2878 to be merged?

LindaSummer · 2025-04-13T13:25:19Z

Hi @SharonIV0x86 ,
I have tested the command line successfully with removing the lock and announcing the command "write" for quick validation after fixing #2878 .
I will try to find a way to improve the performance with less critical section.
Best Regards, Edward

Okay, thanks for looking into it. So, how should i proceed then? should i wait for #2878 to be merged?

Hi @SharonIV0x86 ,

Maybe we could wait for it to be merged before this PR.
It will affect our go integration test cases.

If you don't mind, I will try to solve the lock issue since I find that it may not be so easy to just add a lock key in current connection lock management.
After this PR solved, we could follow it for other commands like CDF.

Best Regards,
Edward

SharonIV0x86 · 2025-04-13T13:30:00Z

Hi @SharonIV0x86 ,

Maybe we could wait for it to be merged before this PR. It will affect our go integration test cases.

If you don't mind, I will try to solve the lock issue since I find that it may not be so easy to just add a lock key in current connection lock management. After this PR solved, we could follow it for other commands like CDF.

Best Regards, Edward

Absolutely no issues whatsoever, there is no hurry i am happy to wait 😄
Let me know if there’s anything I can help with in the meantime.

SharonIV0x86 · 2025-04-16T11:23:06Z

@LindaSummer Hi, i have tested the quantile command after the merging of the #2878 and it works as expected.

However, there is a bit of difference in quantile values when compared to the original implementation of redis tdigest.quantile but i believe kvrocks quantile is more accurate as it uses linear interpolation so it should be good.

I have also added a go test case so if any changes are required pls lmk. 👍🏼

src/commands/cmd_tdigest.cc

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your effort. 😊
Left some comments.

Best Regards,
Edward

LindaSummer · 2025-05-03T07:52:40Z

src/commands/cmd_tdigest.cc

+      return {Status::RedisExecErr, s.ToString()};
+    }
+    if (values_.empty()) {
+      return {Status::RedisExecErr, "invalid quantile or empty tdigest"};


Hi @SharonIV0x86 ,

Sorry for my unclear description for this behavior. 😄

Currently, redis stack returns nan as response for none centroids.
We'd better follow this behavior.

Adding the logic to tdigest ranther than command may be better to keep the command module clean.

After modifying the tdigest, the cpp unit test should also be updated.

Best Regards,
Edward

LindaSummer · 2025-05-03T08:01:19Z

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

 		infoAfterEmptyReset := toTdigestInfo(t, rsp.Val())
 		require.EqualValues(t, 100, infoAfterEmptyReset.Compression)
 	})
+	t.Run("tdigest.quantile with different arguments", func(t *testing.T) {


Hi @SharonIV0x86 ,

We could also add an unordered sequence and an empty tdigest as cases. 😊

Best Regards,
Edward

PragmaTwice · 2025-05-08T05:00:30Z

@LindaSummer Would you like to have a look?

LindaSummer · 2025-05-08T05:26:25Z

@LindaSummer Would you like to have a look?

Hi @PragmaTwice and @SharonIV0x86 ,

Of course!😊

I will go through this PR later today.

Best Regards,
Edward

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your effort! 😊

Left some comments.

Best Regards,
Edward

LindaSummer · 2025-05-08T13:11:17Z

src/commands/cmd_tdigest.cc

-      quantile_strings.push_back(std::to_string(q));
+    if (!result.has_centroids) {
+      for (size_t i = 0; i < values_.size(); ++i) {
+        quantile_strings.emplace_back("nan");


Hi @SharonIV0x86 ,

Maybe we could make "nan" a constexpr literal string in this file. 😊
It would also be used in other commands.

Best Regards,
Edward

Good idea, will do this.

LindaSummer · 2025-05-08T13:13:50Z

src/types/redis_tdigest.cc

  if (auto status = dumpCentroids(ctx, ns_key, metadata, &centroids); !status.ok()) {
    return status;
  }
+  if (centroids.empty()) {


Hi @SharonIV0x86 ,

At the first time we load the metadata, we should know this tdigest has no data inside it.
This could be checked before the centroids retrieving.

And it would be better to add cpp unit test for this behavior.

Best Regards,
Edward

So just to be clear, after we load the metadata here i add a check to do something like this.

if(metadata.merged_nodes == 0 && metadata.unmerged_nodes == 0){ //tdigest does not contain any data. so set the flag }

Correct me if im wrong.

And it would be better to add cpp unit test for this behavior.

I'll try working on this as well.

So just to be clear, after we load the metadata here i add a check to do something like this.

if(metadata.merged_nodes == 0 && metadata.unmerged_nodes == 0){ //tdigest does not contain any data. so set the flag }

Correct me if im wrong.

And it would be better to add cpp unit test for this behavior.

I'll try working on this as well.

Hi @SharonIV0x86 ,

We could directly use total_observations to check the tdigest's elements. 😊

Best Regards,
Edward

SharonIV0x86 · 2025-05-09T07:42:35Z

@LindaSummer I have added the cpp test case, i believe it needs some modifications because in the cpp test case its currently not possible to directly compare the values returned by the quantile function with the "nan" string as the result.quantile vector is returned empty if the tdigest is empty. The behaviors differ in the command implementation.

The closest i could do was to add a assert for the value of result.has_centroids and it should be true and if its found true then it can be assumed that in the Execute function of the command implementation will construct the "nan" strings vector and will return it as a response.

LindaSummer · 2025-05-09T07:53:10Z

@LindaSummer I have added the cpp test case, i believe it needs some modifications because in the cpp test case its currently not possible to directly compare the values returned by the quantile function with the "nan" string as the result.quantile vector is returned empty if the tdigest is empty. The behaviors differ in the command implementation.

The closest i could do was to add a assert for the value of result.has_centroids and it should be true and if its found true then it can be assumed that in the Execute function of the command implementation will construct the "nan" strings vector and will return it as a response.

Hi @SharonIV0x86 ,

You could directly test the flag you have added. 😊
If you want a more clear semantic of the result, maybe std::optional<std::vector<double> is a choice.

And the nan string test could be in integration test.

Best Regards,
Edward

LindaSummer

Hi @SharonIV0x86 ,

Thanks very much for your contribution and huge effort! 😄
I have made some small changes on current code.

LGTM.

Best Regards,
Edward

sonarqubecloud · 2025-05-16T16:24:55Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

SharonIV0x86 · 2025-05-17T05:32:12Z

Hi @SharonIV0x86 ,

Thanks very much for your contribution and huge effort! 😄 I have made some small changes on current code.

LGTM.

Best Regards, Edward

Thanks for the changes you made, the code is much better now with ranges and views.
I have opened a PR for the quantile command in kvrocks-website apache/kvrocks-website#300

SharonIV0x86 and others added 4 commits March 23, 2025 12:46

added command class for quantile command

dc1bb1a

Merge branch 'unstable' into feat/quantile-command

a2e99dc

Implemented QUANTILE command

7bc9b81

Merge branch 'unstable' into feat/quantile-command

a33b2cf

SharonIV0x86 marked this pull request as ready for review March 30, 2025 17:51

danishgh approved these changes Mar 30, 2025

View reviewed changes

Merge branch 'unstable' into feat/quantile-command

562798d

LindaSummer mentioned this pull request Apr 13, 2025

fix(tdigest): fix centroid encoding overwrite for same mean #2878

Merged

mapleFU and others added 5 commits April 13, 2025 22:11

Merge branch 'unstable' into feat/quantile-command

6e0a7f0

Merge branch 'unstable' into feat/quantile-command

64ee2e7

Tested working of TDIGEST.QUANTILE locally.

aa04e94

Added go test case for quantile command

185898f

Fixed return status for non existent key

359f151

SharonIV0x86 and others added 3 commits April 17, 2025 18:55

Merge branch 'unstable' into feat/quantile-command

63c34f3

Merge branch 'unstable' into feat/quantile-command

7937777

Merge branch 'unstable' into feat/quantile-command

42e4667

PragmaTwice reviewed Apr 21, 2025

View reviewed changes

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

PragmaTwice reviewed Apr 21, 2025

View reviewed changes

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

SharonIV0x86 and others added 2 commits April 21, 2025 11:46

Merge branch 'unstable' into feat/quantile-command

d77f5d6

Made suggested changes

b1b951b

LindaSummer reviewed May 3, 2025

View reviewed changes

SharonIV0x86 and others added 3 commits May 6, 2025 21:49

Merge branch 'unstable' into feat/quantile-command

0ecaf4e

Merge branch 'unstable' into feat/quantile-command

8e71585

made suggested check for empty digest and nan response

30538a9

SharonIV0x86 requested a review from LindaSummer May 7, 2025 16:38

Merge branch 'unstable' into feat/quantile-command

ad75911

SharonIV0x86 and others added 3 commits May 8, 2025 12:24

fixed clang-tidy error: used emplace_back instead of push_back

3127abb

gocase for unordered quantiles

f6c3cb5

Merge branch 'unstable' into feat/quantile-command

323d71b

LindaSummer reviewed May 8, 2025

View reviewed changes

SharonIV0x86 and others added 2 commits May 8, 2025 21:04

added 'nan' as a constexpr literal string in cmd_tdigest.cc

f362094

Merge branch 'unstable' into feat/quantile-command

b4dbb7a

SharonIV0x86 requested a review from LindaSummer May 8, 2025 16:45

SharonIV0x86 added 2 commits May 9, 2025 10:29

check for empty tdigest

354f9f3

added cpp unit test case for empty tdigest

4defcd8

SharonIV0x86 and others added 4 commits May 9, 2025 18:49

Merge branch 'unstable' into feat/quantile-command

ad188f5

Merge branch 'unstable' into feat/quantile-command

aba6a5c

added an early check inside lock for empty tdigest

f095c3b

add nan as response for empty tdigest

249564c

LindaSummer approved these changes May 16, 2025

View reviewed changes

PragmaTwice approved these changes May 16, 2025

View reviewed changes

PragmaTwice merged commit 86df0a6 into apache:unstable May 16, 2025
34 checks passed

SharonIV0x86 mentioned this pull request May 17, 2025

Add docs for TDIGEST.QUANTILE command apache/kvrocks-website#300

Merged

SharonIV0x86 deleted the feat/quantile-command branch August 31, 2025 15:56

feat(tdigest): Add support for QUANTILE command #2849

feat(tdigest): Add support for QUANTILE command #2849

Uh oh!

Conversation

SharonIV0x86 commented Mar 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

LindaSummer commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SharonIV0x86 commented Apr 1, 2025

Uh oh!

LindaSummer commented Apr 8, 2025

Uh oh!

SharonIV0x86 commented Apr 8, 2025

Uh oh!

LindaSummer commented Apr 13, 2025

Uh oh!

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

Uh oh!

Uh oh!

LindaSummer commented Apr 13, 2025

Uh oh!

SharonIV0x86 commented Apr 13, 2025

Uh oh!

SharonIV0x86 commented Apr 16, 2025

Uh oh!

Uh oh!

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer May 3, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer May 3, 2025

Choose a reason for hiding this comment

Uh oh!

PragmaTwice commented May 8, 2025

Uh oh!

LindaSummer commented May 8, 2025

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer May 8, 2025

Choose a reason for hiding this comment

Uh oh!

SharonIV0x86 May 8, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer May 8, 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 May 8, 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!

LindaSummer May 9, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

LindaSummer commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented May 16, 2025

Quality Gate passed

Uh oh!

SharonIV0x86 commented May 17, 2025

Uh oh!

Reviewers

SharonIV0x86 commented Mar 30, 2025 •

edited

Loading

LindaSummer commented Apr 1, 2025 •

edited

Loading

SharonIV0x86 commented Apr 13, 2025 •

edited

Loading

LindaSummer May 8, 2025 •

edited

Loading

SharonIV0x86 May 8, 2025 •

edited

Loading

SharonIV0x86 commented May 9, 2025 •

edited

Loading

LindaSummer commented May 9, 2025 •

edited

Loading