feat(tdigest): add the support of TDIGEST.BYRANK and TDIGEST.BYREVRANK command#3296
Conversation
… into refactor/tdigest-rank
Co-authored-by: Edward Xu <xuxiangad@foxmail.com>
… into refactor/tdigest-rank
… into refactor/tdigest-rank
…il inline functions
… into refactor/tdigest-rank
…etter organization
…Rank methods into prepareRankData function
|
Hi @donghao526 , Thanks very much for your great effort! ❤️ I will review this PR in two days. 😊 Best Regards, |
There was a problem hiding this comment.
Pull request overview
This PR implements the TDIGEST.BYRANK and TDIGEST.BYREVRANK commands, which provide the inverse operation of the existing RANK and REVRANK commands - they return the value at a given rank position rather than the rank of a given value. The implementation follows established patterns in the codebase by using template parameters to differentiate between normal and reverse operations, and includes comprehensive test coverage for both C++ and Go test suites.
Key Changes
- Adds core algorithm implementation for retrieving values by rank with support for both forward and reverse iteration
- Implements command handlers with proper input validation (non-negative rank requirement) and deduplication
- Adds comprehensive test coverage including edge cases (empty sketches, duplicates, out-of-range ranks) with support for both RESP2 and RESP3 protocols
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/tdigest.h | Adds TDigestByRank template function implementing the core algorithm for rank-to-value lookup |
| src/types/redis_tdigest.h | Declares ByRank and ByRevRank methods in the TDigest class |
| src/types/redis_tdigest.cc | Implements ByRank and ByRevRank methods with proper error handling for empty sketches |
| src/commands/cmd_tdigest.cc | Adds command handlers for TDIGEST.BYRANK and TDIGEST.BYREVRANK with input validation and deduplication |
| src/commands/error_constants.h | Adds error message constant for invalid (negative) rank values |
| tests/cppunit/types/tdigest_test.cc | Adds C++ unit tests covering empty sketches, populated data, and boundary conditions |
| tests/gocase/unit/type/tdigest/tdigest_test.go | Adds Go tests for empty/non-empty sketches, duplicates, and RESP2/RESP3 protocol handling; enhances toTdigestInfo to support both RESP formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
LindaSummer
left a comment
LindaSummer
left a comment
There was a problem hiding this comment.
Hi @donghao526 ,
Left some comments.
Generally LGTM. 😊
Thanks for your effort!
…ate test assertions for consistency
…r byrank/byrevrank
…d improve error handling
…tions for consistency
Hi @LindaSummer |
got it!🚀 |
LindaSummer
left a comment
LindaSummer
left a comment
There was a problem hiding this comment.
Left one comment.
Generally LGTM.
Thanks very much for your effort! 😊
| TotalCompressions: v["Total compressions"].(int64), | ||
| // Handle both RESP2 (map) and RESP3 (array) formats | ||
| switch v := value.(type) { | ||
| case map[interface{}]interface{}: |
There was a problem hiding this comment.
This seems a little tricky, we'd better pass a parameter explicitly. Or we could provide two versions and let the test case choose which version should use.
There was a problem hiding this comment.
Got it. I've reverted this part since the RESP3 test only applies to byrank commands. It was an oversight that I didn't remove it before.
…6/kvrocks into feature/tdigest-byrank
LindaSummer
left a comment
LindaSummer
left a comment
There was a problem hiding this comment.
Hi @donghao526 ,
LGTM.
Thanks very much for your contribution!❤️
|
This PR close #3064 and #3065