feat(tdigest): add the support of TDIGEST.REVRANK command #3130

donghao526 · 2025-08-19T15:55:41Z

ISSUE

It closes #3063.

Proposed Changes

Add TDIGEST.REVRANK command implementation
Add cpp unit tests

PragmaTwice · 2025-08-19T16:43:13Z

Thank you for your contribution. Could you add some golang test cases for it?

Refer to https://github.com/apache/kvrocks/blob/unstable/tests/gocase/unit/type/tdigest/tdigest_test.go.

donghao526 · 2025-08-19T23:38:56Z

@PragmaTwice ok，I will add some golang test

sonarqubecloud · 2025-08-20T02:52:21Z

Quality Gate failed

Failed conditions
39.7% Coverage on New Code (required ≥ 50%)

See analysis details on SonarQube Cloud

LindaSummer

Hi @donghao526 ,

Thanks very much for your contribution! 😊

Left some comments.

Best Regards,
Edward

LindaSummer · 2025-08-20T07:43:32Z

src/types/redis_tdigest.cc

+  {
+    LockGuard guard(storage_->GetLockManager(), ns_key);
+
+    if (auto status = getMetaDataByNsKey(ctx, ns_key, &metadata); !status.ok()) {
+      return status;
+    }
+
+    if (metadata.total_observations == 0) {
+      result->resize(inputs.size(), -2);
+      return rocksdb::Status::OK();
+    }
+
+    if (metadata.unmerged_nodes > 0) {
+      auto batch = storage_->GetWriteBatchBase();
+      WriteBatchLogData log_data(kRedisTDigest);
+      if (auto status = batch->PutLogData(log_data.Encode()); !status.ok()) {
+        return status;
+      }
+
+      if (auto status = mergeCurrentBuffer(ctx, ns_key, batch, &metadata); !status.ok()) {
+        return status;
+      }
+
+      std::string metadata_bytes;
+      metadata.Encode(&metadata_bytes);
+      if (auto status = batch->Put(metadata_cf_handle_, ns_key, metadata_bytes); !status.ok()) {
+        return status;
+      }
+
+      if (auto status = storage_->Write(ctx, storage_->DefaultWriteOptions(), batch->GetWriteBatch()); !status.ok()) {
+        return status;
+      }
+
+      ctx.RefreshLatestSnapshot();
+    }
+  }


Hi @donghao526 ,

The merge action could be refactored into a function to reduce duplication of the same logic.

Best Regards,
Edward

LindaSummer · 2025-08-20T07:46:38Z

src/types/redis_tdigest.cc

+  for (auto value : inputs) {
+    auto status_or_rank = TDigestRevRank(dump_centroids, value);
+    if (!status_or_rank) {
+      return rocksdb::Status::InvalidArgument(status_or_rank.Msg());
+    }
+    result->push_back(*status_or_rank);
+  }


Hi @donghao526 ,

We could sort the inputs and get the ranks with just one scan of the centroids since it's sorted.

Best Regards,
Edward

Hi @LindaSummer
I encountered a problem when I was testing. After the nodes merged, are there two adjacent centroids can be with the same mean?

I Test with

TDIGEST.CREATE s COMPRESSION 1000 TDIGEST.ADD s 10 10 10 10 20 20

I found the centroids after merged are:
(1) mean: 10 weight: 1
(2) mean: 10 weight: 1
(3) mean: 10 weight: 1
(4) mean: 10 weight: 1
(5) mean: 20 weight: 1
(6) mean: 20 weight: 1

Is this as expected or a bug?

Hi @donghao526 ,

It is expected, and you could refer to #2878 for more details.
So we need a stable way for both serialization and deserialization.

The trigger for the merge is the weight, not the mean. So we could treat the mean only as a label of one centroid. The whole logic is driven by weight.

Best Regards,
Edward

LindaSummer · 2025-08-20T07:52:08Z

src/types/tdigest.h

 }
+
+template <typename TD>
+inline StatusOr<int> TDigestRevRank(TD&& td, double value) {


Hi @donghao526 ,

We need to use a stable way to compare between doubles.

It will be tough to assume that the two double numbers are equal to or greater than.

After solving this, we should add some test cases for this corner case.

Best Regards,
Edward

Hi @donghao526 ,

Since the other code snippets use this way now. You could leave it with the current logic.

I will try to create a new PR to solve the unstable comparison problem in this file.

Best Regards,
Edward

OK, after your new PR, I can help to fix here.

…ks into feature/tdigest-revrank

…apache#3131)

Co-authored-by: hulk <[email protected]>

… commands (apache#3107) Co-authored-by: Twice <[email protected]>

…e#3136) The usage of this function was removed in apache#395. Co-authored-by: Twice <[email protected]>

…pache#3139) Accessing undeclared keys in lua scripting may lead to unexpected behavior in the current design of kvrocks (also in redis, refer to https://redis.io/docs/latest/commands/eval/), so in this PR we add a new option `lua-strict-key-accessing` to prevent users to access undeclared keys in lua, e.g. ``` EVAL "return redis.call('set', 'a', 1)" // ERROR! EVAL "return redis.call('set', KEYS[1], 1)" 1 a // ok ``` This check is performed in both lua scripting and lua functions.

Also, use hardcoded IDs when creating the user and group, to ensure they remain stable. Closes apache#3135. Co-authored-by: Aleks Lozovyuk <[email protected]>

Part of apache#3048

…lt (apache#3144) To align with the description in kvrocks.conf. https://github.com/apache/kvrocks/blob/9446fbde325767f7b64896c4539035402d0f8f7a/kvrocks.conf#L1016-L1017

…()` (apache#3145) Historically, it was used for doing some checks. https://github.com/apache/kvrocks/blob/2bbfe5aa9531f6e76bfd10a2cc450b9bfa0f15d9/src/storage.cc#L175-L182 But these checks no longer exist today, so this operation should be unnecessary. `ListColumnFamilies()` needs to iterate through the MANIFEST, and might be costly for a large one. For example, 80MB MANIFEST, took 1.6 seconds.

…che#3146) Pattern-based SCAN iterations may skip remaining keys in a hash slot, causing incomplete key retrieval. **Bug Reproduction:** - 10 keys in the same hash slot: ["119483", "166988", "210695", "223656", "48063", "59022", "65976", "74937", "88379", "99338"] - Initial SCAN with pattern `2*`: - Returns cursor C1 and **empty keyset** (no keys match `2*`) - Records "119483" as last scanned key - Subsequent SCAN with cursor C1 and same pattern: 1. RocksDB iterator seeks to "119483" 2. Calls `Next()` → gets "166988" (next key in slot) 3. "166988" ∉ `2*` pattern → no key returned 4. **Error**: Scan incorrectly increments slot index 5. **Result**: Remaining 8 keys in slot are skipped **Bug Fix Implementation:** When scanning with a previous scan cursor and match pattern: 1. If the last scanned key is lexicographically before the pattern's start range: → Use the pattern's minimum matching key as the seek key → Instead of using the last scanned key **Example:** - Pattern: `2*` → Minimum matching key = `"2"` (hex: \x32) - Last scanned key: `"119483"` (hex: \x31\x31...) - Since `"119483"` < `"2"` lexically: ✓ **Correct:** Seek to `"2"` ✗ **Buggy:** Seek to `"119483"` --------- Co-authored-by: Twice <[email protected]>

…ks into feature/tdigest-revrank

donghao526 · 2025-08-28T05:29:05Z

@LindaSummer I have modified the code according to your suggestion, please review.

The merge action has been refactored into the mergeNodes function
I have sorted the inputs in TDigestRevRank, now we could get the ranks with just one scan of the centroids

Best Regards

LindaSummer

Hi @donghao526 ,

Thanks very much for your great effort! 😊

Left some comments.

Best Regards,
Edward

LindaSummer · 2025-08-29T01:35:26Z

src/commands/cmd_tdigest.cc

+      return {Status::RedisExecErr, s.ToString()};
+    }
+
+    if (result.data()) {


Suggested change

if (result.data()) {

if (!result.empty()) {

src/types/redis_tdigest.cc

LindaSummer · 2025-08-29T03:25:25Z

src/types/tdigest.h

+      i--;
+
+      // handle the prev inputs which has the same value
+      while ((i > 0) && (inputs[indices[i]] == inputs[indices[i - 1]])) {


Maybe we could use a map<double, std::vector<size_t>> to construct a sorted input and group redundant input with rank into a group, and it may simplify our logic.

LindaSummer · 2025-08-29T03:31:42Z

src/types/tdigest.h

+    i--;
+  }
+  return Status::OK();
+}


Add a newline to the file.

LindaSummer · 2025-08-29T03:41:15Z

src/types/tdigest.h

 }
+
+template <typename TD>
+inline Status TDigestRank(TD&& td, const std::vector<double>& inputs, std::vector<int>& result) {


This function seems specialized for RevRank rather than Rank.

Could we implement the Rank, then wrap the Iterator to a reverse version with the same logic to construct RevRank?

…ks into feature/tdigest-revrank

donghao526 added 7 commits August 17, 2025 22:50

feat: impl tdigest.revrank

03b69e1

feat: impl tdigest.revrank

97df4a1

feat: impl tdigest.revrank

70a39d3

feat: impl tdigest.revrank

dde8410

feat: impl tdigest.revrank

0d3e9cc

test: add unit test for tdigest.revrank

bb172a8

test: add unit test for tdigest.revrank

a64add4

PragmaTwice changed the title ~~feat(tdigest): add TDIGEST.Revrank command implementation #3063~~ feat(tdigest): add the support of TDIGEST.REVRANK command Aug 19, 2025

Merge branch 'unstable' into feature/tdigest-revrank

3954b1f

donghao526 closed this Aug 19, 2025

donghao526 reopened this Aug 19, 2025

donghao526 added 2 commits August 20, 2025 08:39

add golang test cases for tdigest.revrank

05d1202

add golang test cases for tdigest.revrank

f688e14

donghao526 and others added 4 commits August 20, 2025 10:56

add golang test cases for tdigest.revrank

8bcad0f

add golang test cases for tdigest.revrank

46ac984

add golang test cases for tdigest.revrank

495e072

add golang test cases for tdigest.revrank

2b6785d

PragmaTwice requested a review from LindaSummer August 20, 2025 06:52

LindaSummer reviewed Aug 20, 2025

View reviewed changes

donghao526 added 2 commits August 20, 2025 20:58

feat: impl tdigest.revrank

3af3b54

Merge branch 'feature/tmp' into feature/tdigest-revrank

f3d85d3

donghao526 marked this pull request as draft August 20, 2025 14:27

donghao526 and others added 5 commits August 21, 2025 15:11

Merge branch 'unstable' into feature/tdigest-revrank

e68689d

feat: impl tdigest.revrank

eb8674f

Merge branch 'feature/tmp' into feature/tdigest-revrank

c70f410

Merge branch 'unstable' into feature/tdigest-revrank

b991d0d

Merge branch 'feature/tdigest-revrank' of github.com:donghao526/kvroc…

4c9a41d

…ks into feature/tdigest-revrank

zhixinwen and others added 17 commits August 28, 2025 13:06

fix(replication): Fix Seg Fault On Signal When Replication is Enabled (…

543fda0

…apache#3131)

chore(.asf.yaml): make 2.13 a protected branches (apache#3129)

e0d39a7

Co-authored-by: hulk <[email protected]>

feat(ts): Add support for data writing and TS.CREATE, TS.ADD/MADD…

a4ed14c

… commands (apache#3107) Co-authored-by: Twice <[email protected]>

feat(ts): Add TS.INFO command (apache#3133)

9d6c532

chore(.asf.yaml): enable auto merge and disable wiki (apache#3137)

ff658f8

chore: remove unused autoResizeBlockAndSST method and config (apach…

53e82f8

…e#3136) The usage of this function was removed in apache#395. Co-authored-by: Twice <[email protected]>

feat(Dockerfile): add a UID for the user in the container (apache#3138)

201afed

Also, use hardcoded IDs when creating the user and group, to ensure they remain stable. Closes apache#3135. Co-authored-by: Aleks Lozovyuk <[email protected]>

feat(ts): Add data query support and TS.RANGE command (apache#3140)

0851c22

Part of apache#3048

feat(ts): Add TS.GET command (apache#3142)

c7ed36f

Part of apache#3048

chore(config): enable level_compaction_dynamic_level_bytes by defau…

3a898fe

…lt (apache#3144) To align with the description in kvrocks.conf. https://github.com/apache/kvrocks/blob/9446fbde325767f7b64896c4539035402d0f8f7a/kvrocks.conf#L1016-L1017

style: add some comments on TDigestRank

bd268b4

Merge branch 'feature/tdigest-revrank' of github.com:donghao526/kvroc…

8e6a7f9

…ks into feature/tdigest-revrank

refactor: remove commented code

6662240

style: format code

e7f06a2

donghao526 requested a review from LindaSummer August 28, 2025 05:29

donghao526 marked this pull request as ready for review August 28, 2025 05:29

Merge branch 'unstable' into feature/tdigest-revrank

367981c

LindaSummer reviewed Aug 29, 2025

View reviewed changes

donghao526 marked this pull request as draft September 18, 2025 12:10

donghao526 added 4 commits September 18, 2025 20:11

Merge branch 'unstable' into feature/tdigest-revrank

4b8cd6a

feat: sort the input using map in revrank

07836fd

Merge branch 'feature/tdigest-revrank' of github.com:donghao526/kvroc…

f44bc56

…ks into feature/tdigest-revrank

Merge branch 'unstable' into feature/tdigest-revrank

2aded75

feat(tdigest): add the support of TDIGEST.REVRANK command #3130

Are you sure you want to change the base?

feat(tdigest): add the support of TDIGEST.REVRANK command #3130

Uh oh!

Conversation

donghao526 commented Aug 19, 2025 • edited by PragmaTwice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ISSUE

Proposed Changes

Uh oh!

PragmaTwice commented Aug 19, 2025

Uh oh!

donghao526 commented Aug 19, 2025

Uh oh!

sonarqubecloud bot commented Aug 20, 2025

Quality Gate failed

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!

donghao526 Aug 25, 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 Aug 26, 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!

donghao526 commented Aug 28, 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!

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!

8 participants

donghao526 commented Aug 19, 2025 •

edited by PragmaTwice

Loading

donghao526 Aug 25, 2025 •

edited

Loading

LindaSummer Aug 26, 2025 •

edited

Loading