LindaSummer commented on code in PR #3249:
URL: https://github.com/apache/kvrocks/pull/3249#discussion_r2506963937
##########
src/types/redis_tdigest.cc:
##########
@@ -93,11 +101,59 @@ class DummyCentroids {
const std::vector<Centroid>& centroids_;
};
- std::unique_ptr<Iterator> Begin() { return
std::make_unique<Iterator>(centroids_.cbegin(), centroids_); }
- std::unique_ptr<Iterator> End() {
+ class ReverseIterator final : public BaseIterator {
+ public:
+ ReverseIterator(std::vector<Centroid>::const_reverse_iterator&& iter,
const std::vector<Centroid>& centroids)
Review Comment:
Current implementation is ok to me.
If we want a cleaner implementation, maybe we could use crtp to constraint
the `std::vector<Centroid>::const_reverse_iterator` as an compile time
interface with `cbegin()` and `cend()`.
This is optional.
##########
src/commands/cmd_tdigest.cc:
##########
@@ -176,8 +176,9 @@ class CommandTDigestAdd : public Commander {
std::vector<double> values_;
};
-class CommandTDigestRevRank : public Commander {
+class TDigestRankCommand : public Commander {
Review Comment:
Maybe we could use template to dispatch the commands like below.
The `reverse_` could be a compile time constraint.
https://github.com/apache/kvrocks/blob/77b01b6fed23d2763500756d74d9e6e31deb932d/src/commands/cmd_script.cc#L29-L57
##########
src/types/tdigest.h:
##########
@@ -171,67 +172,85 @@ struct DoubleComparator {
};
template <typename TD>
-inline Status TDigestRevRank(TD&& td, const std::vector<double>& inputs,
std::vector<int>& result) {
- std::map<double, size_t, DoubleComparator> value_to_indices;
+inline Status TDigestRank(TD&& td, const std::vector<double>& inputs, bool
reverse, std::vector<int>& result) {
+ std::map<double, size_t, DoubleComparator> value_to_index;
for (size_t i = 0; i < inputs.size(); ++i) {
- value_to_indices[inputs[i]] = i;
+ value_to_index[inputs[i]] = i;
}
result.clear();
result.resize(inputs.size(), -2);
- auto it = value_to_indices.rbegin();
- // handle inputs larger than maximum
- while (it != value_to_indices.rend() && it->first > td.Max()) {
- result[it->second] = -1;
- ++it;
+ using ForwardIter = typename decltype(value_to_index)::iterator;
Review Comment:
Maybe we could make here a `constexpr if` here or a template dispatching.
The `reverse` value could be evaluated in compile time.
##########
tests/gocase/unit/type/tdigest/tdigest_test.go:
##########
@@ -587,4 +587,73 @@ func tdigestTests(t *testing.T, configs
util.KvrocksServerConfigs) {
require.EqualValues(t, rank, expected[i])
}
})
+
Review Comment:
Maybe we could add a case with more different or same and unordered inputs
to have a comprehensive integration test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
