LindaSummer commented on code in PR #2849:
URL: https://github.com/apache/kvrocks/pull/2849#discussion_r2071069820
##########
src/commands/cmd_tdigest.cc:
##########
@@ -242,11 +242,47 @@ class CommandTDigestMax : public CommandTDigestMinMax {
public:
CommandTDigestMax() : CommandTDigestMinMax(false) {}
};
+class CommandTDigestQuantile : public Commander {
+ Status Parse(const std::vector<std::string> &args) override {
+ key_name_ = args[1];
+ values_.reserve(args.size() - 2);
+ for (size_t i = 2; i < args.size(); i++) {
+ auto value = ParseFloat(args[i]);
+ if (!value) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ values_.push_back(*value);
+ }
+ return Status::OK();
+ }
+ Status Execute(engine::Context &ctx, Server *srv, Connection *conn,
std::string *output) override {
+ TDigest tdigest(srv->storage, conn->GetNamespace());
+ TDigestQuantitleResult result;
+ auto s = tdigest.Quantile(ctx, key_name_, values_, &result);
Review Comment:
We should handle the case for an empty tdigest in `TDigest::Quantile`.
Cpp unit test cases should also be adapted after we handle it in
`TDigest::Quantile`.
Here is my test case in redis stack.
```
127.0.0.1:6389> flushall
OK
127.0.0.1:6389> tdigest.create hello
OK
127.0.0.1:6389> tdigest.quantile hello 0.1
1) "nan"
127.0.0.1:6389>
```
Here is the output of current implementation.
```
127.0.0.1:6666> flushall
OK
127.0.0.1:6666> tdigest.create hello
OK
127.0.0.1:6666> tdigest.quantile hello 0.1
(error) ERR Invalid argument: invalid quantile or empty tdigest
```
##########
tests/gocase/unit/type/tdigest/tdigest_test.go:
##########
@@ -310,4 +311,43 @@ func tdigestTests(t *testing.T, configs
util.KvrocksServerConfigs) {
infoAfterEmptyReset := toTdigestInfo(t, rsp.Val())
require.EqualValues(t, 100, infoAfterEmptyReset.Compression)
})
+ t.Run("tdigest.quantile with different arguments", func(t *testing.T) {
+ keyPrefix := "t_qt_"
+
+ //Testing with no arguments
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.QUANTILE").Err(),
errMsgWrongNumberArg)
+
+ // Quantile on a non existent key
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.QUANTILE",
keyPrefix+"iDoNotExist").Err(), errMsgKeyNotExist)
+
+ // Creating a key
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE",
keyPrefix+"01", "compression", "100").Err())
+
+ key := keyPrefix + "01"
+ //Adding some data to tdigest 1 2 2 3 3 3 4 4 4 4 5 5 5 5 5
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", key, "1", "2",
"2", "3", "3", "3", "4", "4", "4", "4", "5", "5", "5", "5", "5").Err())
Review Comment:
Maybe we could add some unordered inputs with some negative numbers as test
cases. 😊
##########
src/commands/cmd_tdigest.cc:
##########
@@ -242,11 +242,47 @@ class CommandTDigestMax : public CommandTDigestMinMax {
public:
CommandTDigestMax() : CommandTDigestMinMax(false) {}
};
+class CommandTDigestQuantile : public Commander {
+ Status Parse(const std::vector<std::string> &args) override {
+ key_name_ = args[1];
+ values_.reserve(args.size() - 2);
+ for (size_t i = 2; i < args.size(); i++) {
+ auto value = ParseFloat(args[i]);
+ if (!value) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ values_.push_back(*value);
+ }
+ return Status::OK();
+ }
+ Status Execute(engine::Context &ctx, Server *srv, Connection *conn,
std::string *output) override {
+ TDigest tdigest(srv->storage, conn->GetNamespace());
+ TDigestQuantitleResult result;
+ auto s = tdigest.Quantile(ctx, key_name_, values_, &result);
+ if (!s.ok()) {
+ if (s.IsNotFound()) {
+ return {Status::RedisExecErr, errKeyNotFound};
+ }
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ std::vector<std::string> quantile_strings;
+ quantile_strings.reserve(result.quantiles.size());
+ for (const auto &q : result.quantiles) {
+ quantile_strings.push_back(std::to_string(q));
+ }
+ *output = conn->MultiBulkString(quantile_strings);
+ return Status::OK();
+ }
+ private:
+ std::string key_name_;
+ std::vector<double> values_;
+};
REDIS_REGISTER_COMMANDS(TDigest,
MakeCmdAttr<CommandTDigestCreate>("tdigest.create", -2, "write", 1, 1, 1),
MakeCmdAttr<CommandTDigestInfo>("tdigest.info", 2,
"read-only", 1, 1, 1),
MakeCmdAttr<CommandTDigestAdd>("tdigest.add", -3,
"write", 1, 1, 1),
MakeCmdAttr<CommandTDigestMax>("tdigest.max", 2,
"read-only", 1, 1, 1),
MakeCmdAttr<CommandTDigestMin>("tdigest.min", 2,
"read-only", 1, 1, 1),
+
MakeCmdAttr<CommandTDigestQuantile>("tdigest.quantile", -2, "read-only", 1, 1,
1),
Review Comment:
```suggestion
MakeCmdAttr<CommandTDigestQuantile>("tdigest.quantile", -3, "read-only", 1, 1,
1),
```
--
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]
