feat(tdigest): implement tdigest.cdf command#3551
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Redis-compatible support for the TDIGEST.CDF command to Kvrocks’ TDigest feature set (issue #2807), along with new C++ unit tests and Go integration tests to validate behavior against Redis.
Changes:
- Add
TDigest::CDFAPI and implementation in the TDigest type layer. - Register and implement the
tdigest.cdfcommand handler. - Add Go and C++ tests covering argument validation, empty-digest behavior, and sample-based correctness.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gocase/unit/type/tdigest/tdigest_test.go | Adds Go integration tests for TDIGEST.CDF including empty/non-empty digests and Redis-sourced samples. |
| tests/cppunit/types/tdigest_test.cc | Adds C++ unit tests for TDigest::CDF across several distributions and edge cases. |
| src/types/redis_tdigest.h | Introduces TDigestCDFResult and the TDigest::CDF API. |
| src/types/redis_tdigest.cc | Implements the TDigest::CDF computation over stored centroids/buffers. |
| src/commands/cmd_tdigest.cc | Adds the tdigest.cdf command implementation and registers it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Anirudh Lakhanpal <anirudhlakhanpal123@gmail.com>
507af23 to
0fdd2bf
Compare
jihuayu
left a comment
There was a problem hiding this comment.
The CDF algorithm in this PR differs from Redis’s implementation. Was this intentional?
TDIGEST.CREATE t COMPRESSION 10
TDIGEST.ADD t 0 1 2 ... 99
TDIGEST.CDF t 20 40 50 60 80
| Query | RedisBloom | this PR |
|---|---|---|
| 20 | 0.205 | 0.09 |
| 40 | 0.405 | 0.33 |
| 50 | 0.505 | 0.63 |
| 60 | 0.605 | 0.63 |
| 80 | 0.805 | 0.88 |
Hi @jihuayu , Thanks very much for your checking! ❤️ I will investigate the problem and find the root cause. |
|
Hi @jihuayu , Thanks very much for your correction and review. This problem is caused by the missing of interpolation of CDF value. == compressed centroids setup ==
$ redis-cli -h 127.0.0.1 -p 6666 TDIGEST.CREATE kvrocks_check-1:compressed COMPRESSION 10
OK
== compressed centroids add 0..99 ==
$ redis-cli -h 127.0.0.1 -p 6666 TDIGEST.ADD kvrocks_check-1:compressed 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99
OK
== compressed centroids cdf, expected Redis-like: 0.205 0.405 0.505 0.605 0.805 ==
$ redis-cli -h 127.0.0.1 -p 6666 TDIGEST.CDF kvrocks_check-1:compressed 20 40 50 60 80
0.20499999999999999
0.40500000000000003
0.505
0.60499999999999998
0.80500000000000005 |
|
@LindaSummer It doesn’t seem to be aligned. (We have two '7')
|
|
Hi @jihuayu , Thanks for your review and patience! ❤️ I will fix this problem and add more cases to full cover the core logic. |



Issue
Close #2807
Proposed Changes
tdigest.cdfcommand