LindaSummer commented on code in PR #2826:
URL: https://github.com/apache/kvrocks/pull/2826#discussion_r2000215505
##########
src/types/redis_tdigest.cc:
##########
@@ -238,7 +238,50 @@ rocksdb::Status TDigest::Quantile(engine::Context& ctx,
const Slice& digest_name
return rocksdb::Status::OK();
}
+rocksdb::Status TDigest::Reset(engine::Context& ctx, const Slice& digest_name)
{
+ auto ns_key = AppendNamespacePrefix(digest_name);
+
+ TDigestMetadata metadata;
+ auto status = getMetaDataByNsKey(ctx, ns_key, &metadata);
+ if (!status.ok()) {
+ return status;
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisTDigest);
+ status = batch->PutLogData(log_data.Encode());
+ if (!status.ok()) {
+ return status;
+ }
+ metadata.unmerged_nodes = 0;
+ metadata.merged_nodes = 0;
+ metadata.total_weight = 0;
+ metadata.merged_weight = 0;
+ metadata.minimum = std::numeric_limits<double>::max();
+ metadata.maximum = std::numeric_limits<double>::lowest();
+ metadata.total_observations = 0;
+ metadata.merge_times = 0;
+
+ std::string metadata_bytes;
+ metadata.Encode(&metadata_bytes);
+
+ status = batch->Put(metadata_cf_handle_, ns_key, metadata_bytes);
+ if (!status.ok()) {
+ return status;
+ }
+
+ if (!status.ok()) return status;
Review Comment:
it'll be better to keep same code style in same commit.😊
##########
src/types/redis_tdigest.cc:
##########
@@ -238,7 +238,50 @@ rocksdb::Status TDigest::Quantile(engine::Context& ctx,
const Slice& digest_name
return rocksdb::Status::OK();
}
+rocksdb::Status TDigest::Reset(engine::Context& ctx, const Slice& digest_name)
{
+ auto ns_key = AppendNamespacePrefix(digest_name);
+
+ TDigestMetadata metadata;
+ auto status = getMetaDataByNsKey(ctx, ns_key, &metadata);
+ if (!status.ok()) {
+ return status;
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisTDigest);
+ status = batch->PutLogData(log_data.Encode());
+ if (!status.ok()) {
+ return status;
+ }
+ metadata.unmerged_nodes = 0;
+ metadata.merged_nodes = 0;
+ metadata.total_weight = 0;
+ metadata.merged_weight = 0;
+ metadata.minimum = std::numeric_limits<double>::max();
+ metadata.maximum = std::numeric_limits<double>::lowest();
+ metadata.total_observations = 0;
+ metadata.merge_times = 0;
+
+ std::string metadata_bytes;
+ metadata.Encode(&metadata_bytes);
+
+ status = batch->Put(metadata_cf_handle_, ns_key, metadata_bytes);
+ if (!status.ok()) {
+ return status;
+ }
+
+ if (!status.ok()) return status;
+ auto start_key = internalSegmentGuardPrefixKey(metadata, ns_key,
SegmentType::kBuffer);
+ auto guard_key = internalSegmentGuardPrefixKey(metadata, ns_key,
SegmentType::kGuardFlag);
+
+ status = batch->DeleteRange(cf_handle_, start_key, guard_key);
+ if (!status.ok()) return status;
Review Comment:
same as above.
##########
src/types/redis_tdigest.cc:
##########
@@ -238,7 +238,50 @@ rocksdb::Status TDigest::Quantile(engine::Context& ctx,
const Slice& digest_name
return rocksdb::Status::OK();
}
+rocksdb::Status TDigest::Reset(engine::Context& ctx, const Slice& digest_name)
{
+ auto ns_key = AppendNamespacePrefix(digest_name);
+
+ TDigestMetadata metadata;
+ auto status = getMetaDataByNsKey(ctx, ns_key, &metadata);
+ if (!status.ok()) {
+ return status;
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisTDigest);
+ status = batch->PutLogData(log_data.Encode());
+ if (!status.ok()) {
+ return status;
+ }
+ metadata.unmerged_nodes = 0;
+ metadata.merged_nodes = 0;
+ metadata.total_weight = 0;
+ metadata.merged_weight = 0;
+ metadata.minimum = std::numeric_limits<double>::max();
+ metadata.maximum = std::numeric_limits<double>::lowest();
+ metadata.total_observations = 0;
+ metadata.merge_times = 0;
+
+ std::string metadata_bytes;
+ metadata.Encode(&metadata_bytes);
+
+ status = batch->Put(metadata_cf_handle_, ns_key, metadata_bytes);
+ if (!status.ok()) {
+ return status;
+ }
+
+ if (!status.ok()) return status;
+ auto start_key = internalSegmentGuardPrefixKey(metadata, ns_key,
SegmentType::kBuffer);
+ auto guard_key = internalSegmentGuardPrefixKey(metadata, ns_key,
SegmentType::kGuardFlag);
+
+ status = batch->DeleteRange(cf_handle_, start_key, guard_key);
+ if (!status.ok()) return status;
+ status = storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch());
+ if (!status.ok()) return status;
Review Comment:
same as above.
##########
tests/gocase/unit/type/tdigest/tdigest_test.go:
##########
@@ -257,4 +257,41 @@ func tdigestTests(t *testing.T, configs
util.KvrocksServerConfigs) {
require.NoError(t, rsp.Err())
require.Equal(t, "-10.5", rsp.Val())
})
+ t.Run("tdigest.reset with different arguments", func(t *testing.T) {
+ keyPrefix := "tdigest_reset_"
+
+ //testing with no arguments to .RESET
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.RESET").Err(),
errMsgWrongNumberArg)
+
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE",
keyPrefix+"mydigest", "compression", "101").Err())
+
+ key := keyPrefix + "mydigest"
+ //adding some data to digest
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", key, "-84.3",
"199.3", "343.34", "12.34").Err())
+ rsp := rdb.Do(ctx, "TDIGEST.MIN", key)
+ require.NoError(t, rsp.Err())
+ require.EqualValues(t, rsp.Val(), "-84.3")
+
+ //reset on a non existent key.
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.RESET",
keyPrefix+"notexist").Err(), errMsgKeyNotExist)
+ {
Review Comment:
maybe we could add braces for all cases or none to keep same style.😊
##########
tests/gocase/unit/type/tdigest/tdigest_test.go:
##########
@@ -257,4 +257,41 @@ func tdigestTests(t *testing.T, configs
util.KvrocksServerConfigs) {
require.NoError(t, rsp.Err())
require.Equal(t, "-10.5", rsp.Val())
})
+ t.Run("tdigest.reset with different arguments", func(t *testing.T) {
+ keyPrefix := "tdigest_reset_"
+
+ //testing with no arguments to .RESET
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.RESET").Err(),
errMsgWrongNumberArg)
+
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE",
keyPrefix+"mydigest", "compression", "101").Err())
+
+ key := keyPrefix + "mydigest"
+ //adding some data to digest
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", key, "-84.3",
"199.3", "343.34", "12.34").Err())
+ rsp := rdb.Do(ctx, "TDIGEST.MIN", key)
+ require.NoError(t, rsp.Err())
+ require.EqualValues(t, rsp.Val(), "-84.3")
+
+ //reset on a non existent key.
+ require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.RESET",
keyPrefix+"notexist").Err(), errMsgKeyNotExist)
+ {
+ // reset on non-empty digest.
+ require.NoError(t, rdb.Do(ctx, "TDIGEST.RESET",
key).Err())
+ // getting TDIGEST.INFO after an reset for testing
metadata.
+ rsp = rdb.Do(ctx, "TDIGEST.INFO", key)
+ require.NoError(t, rsp.Err())
+ info := toTdigestInfo(t, rsp.Val())
+ require.GreaterOrEqual(t, int64(1024), info.Capacity)
Review Comment:
maybe we could call tdigest.info to get the capacity before reset and assert
it keeps the original value.😊
--
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]
