Copilot commented on code in PR #3551:
URL: https://github.com/apache/kvrocks/pull/3551#discussion_r3560685376
##########
src/commands/cmd_tdigest.cc:
##########
@@ -556,6 +559,65 @@ class CommandTDigestTrimmedMean : public Commander {
double high_cut_quantile_;
};
+class CommandTDigestCDF : public Commander {
+ Status Parse(const std::vector<std::string> &args) override {
+ if (args.size() == 2) return {Status::RedisParseErr,
errWrongNumOfArguments};
+ key_name_ = args[1];
+ std::map<double, std::vector<size_t>> unique_inputs;
+ for (size_t i = 2; i < args.size(); i++) {
+ auto value = ParseFloat(args[i]);
+ if (!value) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ if (std::isnan(*value)) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ if (unique_inputs.find(*value) == unique_inputs.cend()) {
+ unique_inputs[*value] = std::vector<size_t>{i - 2};
+ } else {
+ unique_inputs[*value].push_back(i - 2);
+ }
+ }
+ sorted_unique_inputs_with_idx_ =
+ ranges::views::transform(unique_inputs,
+ [](const auto &pair) { return
std::make_pair(pair.first, pair.second); }) |
+ ranges::to_vector;
+ num_inputs_ = args.size() - 2;
+ return Status::OK();
+ }
+
+ Status Execute(engine::Context &ctx, Server *srv, Connection *conn,
std::string *output) override {
+ TDigest tdigest(srv->storage, conn->GetNamespace());
+ TDigestCDFResult result;
+ std::vector<double> uniq_cdfs = sorted_unique_inputs_with_idx_ |
ranges::views::keys | ranges::to_vector;
+ auto s = tdigest.CDFUniqSorted(ctx, key_name_, uniq_cdfs, &result);
+ if (!s.ok()) {
+ if (s.IsNotFound()) {
+ return {Status::RedisExecErr, errKeyNotFound};
+ }
+ return {Status::RedisExecErr, s.ToString()};
+ }
+
+ std::vector<std::pair<size_t, std::string>> cdf_uniq_results_with_idx =
+ ranges::views::transform(result.cdf_values, util::Float2String) |
ranges::views::enumerate | ranges::to_vector;
+ std::vector<std::string> cdf_result(num_inputs_, kNan);
+
+ for (const auto &[idx, result_str] : cdf_uniq_results_with_idx) {
+ for (const auto &origin_idx :
sorted_unique_inputs_with_idx_[idx].second) {
+ cdf_result[origin_idx] = result_str;
+ }
+ }
Review Comment:
`ranges::views::enumerate | ranges::to_vector` does not reliably produce a
`std::vector<std::pair<size_t, std::string>>` across range-v3 versions (often
it yields a tuple/common_pair). This risks a compilation failure and is
unnecessary here; a simple indexed loop is clearer and avoids the extra
allocation.
##########
src/types/redis_tdigest.cc:
##########
@@ -570,6 +571,94 @@ rocksdb::Status TDigest::Merge(engine::Context& ctx, const
Slice& dest_digest,
return storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch());
}
+rocksdb::Status TDigest::CDFUniqSorted(engine::Context& ctx, const Slice&
digest_name,
+ const std::vector<double>& inputs,
TDigestCDFResult* result) {
+ if (!std::is_sorted(inputs.cbegin(), inputs.cend())) {
+ return rocksdb::Status::InvalidArgument(
+ "Internal error: inputs must be sorted in ascending order for CDF
computation.");
+ }
+
+ if (std::set<double>(inputs.cbegin(), inputs.cend()).size() !=
inputs.size()) {
+ return rocksdb::Status::InvalidArgument("Internal error: inputs must be
unique for CDF computation.");
+ }
Review Comment:
The uniqueness check uses `std::set<double>(...)`. Using floating-point keys
(especially if a NaN ever slips through) violates the strict-weak-ordering
requirement of associative containers and can result in undefined behavior.
Since inputs are already required to be sorted, you can check duplicates with
`std::adjacent_find` in O(n) without any ordering container.
##########
src/commands/cmd_tdigest.cc:
##########
@@ -18,13 +18,16 @@
*
*/
+#include <range/v3/action/sort.hpp>
#include <range/v3/range/conversion.hpp>
+#include <range/v3/view/enumerate.hpp>
#include <range/v3/view/transform.hpp>
Review Comment:
New includes are a bit inconsistent and add unused headers:
`<range/v3/action/sort.hpp>` and `<range/v3/view/enumerate.hpp>` aren’t used,
and `"range/v3/view/map.hpp"` differs from the existing `<range/v3/...>` style
(and can be sensitive to include search order). Consider trimming to only the
needed range-v3 headers and using the `<...>` form.
--
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]
