refactor(tdigest): convert the flags for reverse in TDigest::Rank to a compile-time argument by donghao526 · Pull Request #3268 · apache/kvrocks

donghao526 · 2025-11-20T14:45:45Z

The reverse flag in the current TDigest::Rank implementation is a runtime value. Therefore, it should be converted to a compile-time argument.

PragmaTwice · 2025-11-21T08:11:07Z

+    const std::vector<Centroid>& centroids_;
+  };
+
+  class ReverseIterator final : public BaseIterator {


Could we have something like template <bool Reverse> class Iterator instead of virtual functions?

Of course, I have refactored the code.

… into refactor/tdigest-rank

LindaSummer · 2025-11-22T13:09:29Z

Hi @donghao526 ,

Thanks very much for your effort! ❤
I will go through this PR in three days.

Best Regards,
Edward

…r iterator type

donghao526 · 2025-11-23T10:14:38Z

Hi @donghao526 ,

Thanks very much for your effort! ❤ I will go through this PR in three days.

Best Regards, Edward

Hi @LindaSummer

Thanks a lot. You can review the code at your convenience.😊

LindaSummer

Hi @donghao526 ,

Thanks very much for your contribution! ❤

Generally LGTM.

Left some comments.

Best Regards,
Edward

LindaSummer · 2025-11-23T14:31:14Z

+    Iterator(IterType iter, const std::vector<Centroid>& centroids) : iter_(iter), centroids_(centroids) {}
+    std::unique_ptr<Iterator> Clone() const {
+      if constexpr (Reverse) {
+        if (iter_ != centroids_.crend()) {


Maybe we could add a private method like below.

class Ietrator { ... private: template <typename Container> decltype(auto) get_cbegin_iter(Container centroids) const { if constexpr (Reverse) { return centroids.rcbegin(); } else { return centroids.cbegin() ; } } template <typename Container> decltype(auto) get_cend_iter(Container centroids) const { if constexpr (Reverse) { return centroids.rcend(); } else { return centroids.cend() ; } }

Then we could unify the centroids.cebgin() and centroids.cend().
This could help us reduce the constexpr if in every iterator getter.

Good suggestion, I will refactor it.
Thanks!😊

LindaSummer · 2025-11-23T14:36:03Z

-    return TDigestRankImpl<TD, false>(std::forward<TD>(td), inputs, result);
-  }
-}
+}


Add an endline for this file.

Co-authored-by: Edward Xu <xuxiangad@foxmail.com>

…mprove clarity

… into refactor/tdigest-rank

…o private

PragmaTwice · 2025-11-24T13:02:27Z

+template <bool Reverse>
+rocksdb::Status TDigest::Rank(engine::Context& ctx, const Slice& digest_name, const std::vector<double>& inputs,


I think maybe we can just expose two non-template function TDigest::Rank and TDigest::RevRank so that we can put the definition of this function into the .cc file instead of header?

Not necesary in this PR.

PragmaTwice

Generally looks good. I'll leave it to @LindaSummer.

LindaSummer

Hi @donghao526 ,

Thanks very much for your contribution! ❤

LGTM. Left some optional nitpick comments.

Best Regards,
Edward

LindaSummer · 2025-11-27T15:51:51Z

+  };
+
+  std::unique_ptr<Iterator> Begin() const {
+    if constexpr (Reverse) {


Suggested change

if constexpr (Reverse) {

return std::make_unique<Iterator>(getCbeginIter(centroids_), centroids_);

Maybe here could also use the util.

As the Begin and End functions are outside the Iterator class, I made getCbeginIter and getCendIter static. Is this a good solution?

I think the getbegin and getend are both stateless, static is ok.
maybe they could be a free inline function protected by a detail private namespace if you prefer this way.

LindaSummer · 2025-11-27T15:52:42Z

+  }
+  std::unique_ptr<Iterator> End() const {
+    if (centroids_.empty()) {
+      if constexpr (Reverse) {


Same as above.

…n DummyCentroids

… into refactor/tdigest-rank

…il inline functions

LindaSummer

Hi @donghao526 ,

Thanks very much for your contribution!❤

LGTM, left one comment for namespace design.

Best Regards,
Edward

LindaSummer · 2025-12-03T13:55:05Z


 namespace redis {
+
+namespace detail {


To reduce conflict with other symbols in redis namespace, it would be better guarded by a namespace like tdigest.

After we refactor this in cc file, this could just be in an anonymous namespace.

Hi @LindaSummer

I have exposed two non-template functions, TDigest::Rank and TDigest::RevRank, and placed their definitions in the .cc file.

I have placed the GetCBeginIter and GetCEndIter functions inside an anonymous namespace.

Thanks a lot!

…nk methods

… into refactor/tdigest-rank

…etter organization

…Rank methods into prepareRankData function

…rn value

…f statement

… into refactor/tdigest-rank

LindaSummer

Hi @donghao526 ,

Thanks very much for your contribution! ❤

LGTM

Best Regards,
Edward

donghao526 · 2025-12-07T07:26:14Z

Hi @donghao526 ,

Thanks very much for your contribution! ❤

LGTM

Best Regards,
Edward

Thanks for your review😊

LindaSummer · 2025-12-10T06:03:38Z

Hi @donghao526 ,

The CI has some issue and I'm working on #3290 .

This PR will be merged after this issue solved.

Best Regards,
Edward

sonarqubecloud · 2025-12-13T13:05:12Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

refactor: make all the flags for reverse as a compile-time argument

4784442

aleksraiden requested review from LindaSummer, PragmaTwice and git-hulk November 20, 2025 15:48

Merge branch 'unstable' into refactor/tdigest-rank

45281c3

PragmaTwice reviewed Nov 21, 2025

View reviewed changes

Comment thread src/types/tdigest.h Outdated

PragmaTwice reviewed Nov 21, 2025

View reviewed changes

Comment thread src/types/redis_tdigest.h Outdated

PragmaTwice reviewed Nov 21, 2025

View reviewed changes

Comment thread src/types/redis_tdigest.h Outdated

PragmaTwice reviewed Nov 21, 2025

View reviewed changes

donghao526 added 2 commits November 21, 2025 16:47

refactor(tdigest): change variable reverse to Reverse

b421adb

Merge branch 'refactor/tdigest-rank' of github.com:donghao526/kvrocks…

1ced14d

… into refactor/tdigest-rank

donghao526 added 2 commits November 23, 2025 13:48

refactor(tdigest): update TDigestRank template parameters for clarity

70ec5b8

refactor(tdigest): update DummyCentroids to use template parameter fo…

c42ed44

…r iterator type

LindaSummer reviewed Nov 23, 2025

View reviewed changes

donghao526 and others added 5 commits November 24, 2025 09:40

style: use the init-statement for calling TDigestRank

5d3a885

Co-authored-by: Edward Xu <xuxiangad@foxmail.com>

refactor(tdigest): simplify iterator handling in DummyCentroids and i…

3bb2fe6

…mprove clarity

Merge branch 'refactor/tdigest-rank' of github.com:donghao526/kvrocks…

f1c559c

… into refactor/tdigest-rank

refactor(tdigest): change get_cbegin_iter and get_cend_iter methods t…

7fb6b2f

…o private

refactor(tdigest): rename function names for consistency

0ed408d

PragmaTwice reviewed Nov 24, 2025

View reviewed changes

PragmaTwice requested a review from LindaSummer November 26, 2025 05:43

Merge branch 'unstable' into refactor/tdigest-rank

d5cce3f

LindaSummer previously approved these changes Nov 27, 2025

View reviewed changes

donghao526 added 2 commits December 2, 2025 11:00

refactor(tdigest): make iterator methods static and improve clarity i…

843af3d

…n DummyCentroids

Merge branch 'refactor/tdigest-rank' of github.com:donghao526/kvrocks…

3c935d8

… into refactor/tdigest-rank

donghao526 dismissed LindaSummer’s stale review via 3c935d8 December 2, 2025 03:01

Merge branch 'unstable' into refactor/tdigest-rank

48c1f47

donghao526 added 2 commits December 2, 2025 11:35

refactor(tdigest): rename iterator methods for consistency and clarity

4bdb79b

refactor(tdigest): refactor the GetCbeginIter and GetCendIter to deta…

b6074af

…il inline functions

donghao526 requested a review from LindaSummer December 3, 2025 13:13

LindaSummer reviewed Dec 3, 2025

View reviewed changes

donghao526 added 12 commits December 4, 2025 20:39

Merge branch 'unstable' into refactor/tdigest-rank

c2894a0

refactor(tdigest): replace template Rank with separate Rank and RevRa…

7487397

…nk methods

Merge branch 'refactor/tdigest-rank' of github.com:donghao526/kvrocks…

0aaa788

… into refactor/tdigest-rank

refactor(tdigest): move Rank and RevRank methods above Quantile for b…

be9a404

…etter organization

refactor(tdigest): remove unused RankImpl template function

8f229ce

refactor(tdigest): extract common preparation logic from Rank and Rev…

e73eae4

…Rank methods into prepareRankData function

refactor(tdigest): remove unnecessary comment indicating special retu…

69bf01a

…rn value

refactor(tdigest): fix compile error

e1ed70a

refactor(tdigest): fix clang-format error

798a69d

refactor(tdigest): fix error when get rank on an empty tdigest

1c3ae7d

refactor(tdigest): fix some compile error

4b37eed

Merge branch 'unstable' into refactor/tdigest-rank

6911242

donghao526 requested a review from LindaSummer December 5, 2025 13:13

donghao526 added 2 commits December 6, 2025 20:03

refactor(tdigest): use the init-statement to declare "s" inside the i…

c097f8c

…f statement

Merge branch 'refactor/tdigest-rank' of github.com:donghao526/kvrocks…

f4e5488

… into refactor/tdigest-rank

LindaSummer approved these changes Dec 7, 2025

View reviewed changes

LindaSummer enabled auto-merge (squash) December 7, 2025 05:15

donghao526 requested a review from PragmaTwice December 7, 2025 11:10

Merge branch 'unstable' into refactor/tdigest-rank

a06db87

PragmaTwice approved these changes Dec 13, 2025

View reviewed changes

LindaSummer merged commit 65db00a into apache:unstable Dec 13, 2025
35 checks passed

donghao526 deleted the refactor/tdigest-rank branch December 14, 2025 03:29

		template <bool Reverse>
		rocksdb::Status TDigest::Rank(engine::Context& ctx, const Slice& digest_name, const std::vector<double>& inputs,

	if constexpr (Reverse) {
	return std::make_unique<Iterator>(getCbeginIter(centroids_), centroids_);

Conversation

donghao526 commented Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PragmaTwice Nov 21, 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!

LindaSummer commented Nov 22, 2025

Uh oh!

donghao526 commented Nov 23, 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!

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!

PragmaTwice left a comment

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!

LindaSummer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donghao526 Dec 5, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

donghao526 commented Dec 7, 2025

Uh oh!

LindaSummer commented Dec 10, 2025

Uh oh!

Uh oh!

sonarqubecloud Bot commented Dec 13, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PragmaTwice Nov 21, 2025 •

edited

Loading

donghao526 commented Nov 23, 2025 •

edited

Loading

donghao526 Dec 5, 2025 •

edited

Loading