Copilot commented on code in PR #3551:
URL: https://github.com/apache/kvrocks/pull/3551#discussion_r3572303893
##########
src/types/redis_tdigest.cc:
##########
@@ -570,6 +573,109 @@ rocksdb::Status TDigest::Merge(engine::Context& ctx,
const Slice& dest_digest,
return storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch());
}
+rocksdb::Status TDigest::CDF(engine::Context& ctx, const Slice& digest_name,
const std::vector<double>& inputs,
+ TDigestCDFResult* result) {
+ std::map<double, std::vector<size_t>> sorted_unique_inputs_with_idx;
+ for (size_t i = 0; i < inputs.size(); ++i) {
+ sorted_unique_inputs_with_idx[inputs[i]].push_back(i);
+ }
+
+ std::vector<double> cdf_values;
+ if (auto status = cdfUniqSorted(ctx, digest_name,
sorted_unique_inputs_with_idx | ranges::views::keys | ranges::to_vector,
&cdf_values); !status.ok()) {
+ return status;
+ }
+ result->cdf_values.resize(inputs.size(),
std::numeric_limits<double>::quiet_NaN());
+
+ auto idx = 0;
+ for (auto iter = sorted_unique_inputs_with_idx.cbegin(); iter !=
sorted_unique_inputs_with_idx.cend(); ++iter) {
+ for (auto original_idx : iter->second) {
+ result->cdf_values[original_idx] = cdf_values[idx];
+ }
+ ++idx;
+ }
+
+ return rocksdb::Status::OK();
+}
+
+rocksdb::Status TDigest::cdfUniqSorted(engine::Context& ctx, const Slice&
digest_name,
+ const std::vector<double>& inputs,
std::vector<double>* values) {
+ auto ns_key = AppendNamespacePrefix(digest_name);
+ TDigestMetadata metadata;
+ {
+ LockGuard guard(storage_->GetLockManager(), ns_key);
+
+ if (auto status = getMetaDataByNsKey(ctx, ns_key, &metadata);
!status.ok()) {
+ return status;
+ }
+
+ if (metadata.total_observations == 0) {
+ *values = std::vector<double>(inputs.size(),
std::numeric_limits<double>::quiet_NaN());
+ return rocksdb::Status::OK();
+ }
+
+ if (metadata.unmerged_nodes > 0) {
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisTDigest);
+ if (auto status = batch->PutLogData(log_data.Encode()); !status.ok()) {
+ return status;
+ }
+
+ if (auto status = mergeCurrentBuffer(ctx, ns_key, batch, &metadata);
!status.ok()) {
+ return status;
+ }
+
+ std::string metadata_bytes;
+ metadata.Encode(&metadata_bytes);
+ if (auto status = batch->Put(metadata_cf_handle_, ns_key,
metadata_bytes); !status.ok()) {
+ return status;
+ }
+
+ if (auto status = storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch()); !status.ok()) {
+ return status;
+ }
+ ctx.RefreshLatestSnapshot();
+ }
Review Comment:
`cdfUniqSorted` duplicates the existing `mergeNodes()` logic (write batch +
metadata update + snapshot refresh). This makes CDF’s merge behavior easier to
accidentally diverge from Rank/Quantile over time. Reuse `mergeNodes(ctx,
ns_key, &metadata)` here instead of re-implementing it.
##########
src/types/redis_tdigest.cc:
##########
@@ -570,6 +573,109 @@ rocksdb::Status TDigest::Merge(engine::Context& ctx,
const Slice& dest_digest,
return storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch());
}
+rocksdb::Status TDigest::CDF(engine::Context& ctx, const Slice& digest_name,
const std::vector<double>& inputs,
+ TDigestCDFResult* result) {
+ std::map<double, std::vector<size_t>> sorted_unique_inputs_with_idx;
+ for (size_t i = 0; i < inputs.size(); ++i) {
+ sorted_unique_inputs_with_idx[inputs[i]].push_back(i);
+ }
+
+ std::vector<double> cdf_values;
+ if (auto status = cdfUniqSorted(ctx, digest_name,
sorted_unique_inputs_with_idx | ranges::views::keys | ranges::to_vector,
&cdf_values); !status.ok()) {
+ return status;
+ }
+ result->cdf_values.resize(inputs.size(),
std::numeric_limits<double>::quiet_NaN());
+
+ auto idx = 0;
+ for (auto iter = sorted_unique_inputs_with_idx.cbegin(); iter !=
sorted_unique_inputs_with_idx.cend(); ++iter) {
+ for (auto original_idx : iter->second) {
+ result->cdf_values[original_idx] = cdf_values[idx];
+ }
+ ++idx;
+ }
+
+ return rocksdb::Status::OK();
+}
+
+rocksdb::Status TDigest::cdfUniqSorted(engine::Context& ctx, const Slice&
digest_name,
+ const std::vector<double>& inputs,
std::vector<double>* values) {
+ auto ns_key = AppendNamespacePrefix(digest_name);
+ TDigestMetadata metadata;
+ {
+ LockGuard guard(storage_->GetLockManager(), ns_key);
+
+ if (auto status = getMetaDataByNsKey(ctx, ns_key, &metadata);
!status.ok()) {
+ return status;
+ }
+
+ if (metadata.total_observations == 0) {
+ *values = std::vector<double>(inputs.size(),
std::numeric_limits<double>::quiet_NaN());
+ return rocksdb::Status::OK();
+ }
+
+ if (metadata.unmerged_nodes > 0) {
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisTDigest);
+ if (auto status = batch->PutLogData(log_data.Encode()); !status.ok()) {
+ return status;
+ }
+
+ if (auto status = mergeCurrentBuffer(ctx, ns_key, batch, &metadata);
!status.ok()) {
+ return status;
+ }
+
+ std::string metadata_bytes;
+ metadata.Encode(&metadata_bytes);
+ if (auto status = batch->Put(metadata_cf_handle_, ns_key,
metadata_bytes); !status.ok()) {
+ return status;
+ }
+
+ if (auto status = storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch()); !status.ok()) {
+ return status;
+ }
+ ctx.RefreshLatestSnapshot();
+ }
+ }
+
+ std::vector<Centroid> centroids;
+ if (auto status = dumpCentroids(ctx, ns_key, metadata, ¢roids);
!status.ok()) {
+ return status;
+ }
+
+ auto dump_centroids = DummyCentroids<false>(metadata, centroids);
+ auto total_weight = dump_centroids.TotalWeight();
+ auto iter = dump_centroids.Begin();
+ double accum_weight = 0.;
+ std::vector<double> results;
+ results.reserve(inputs.size());
+ for (const auto val : inputs) {
+ double weight = accum_weight;
+ for (; iter->Valid(); iter->Next()) {
+ auto current_centroid_result = iter->GetCentroid();
+ if (!current_centroid_result) {
+ return rocksdb::Status::InvalidArgument(current_centroid_result.Msg());
+ }
+ auto& current_centroid = *current_centroid_result;
+ if (val < current_centroid.mean) {
+ break;
+ }
+ accum_weight += current_centroid.weight;
+ if (val > current_centroid.mean) {
+ weight += current_centroid.weight;
+ continue;
+ }
+ if (val == current_centroid.mean) {
+ weight += current_centroid.weight / 2;
+ continue;
+ }
Review Comment:
CDF’s centroid comparisons use raw `<`, `>`, and `==` on doubles, unlike
other tdigest operations (e.g. Rank) which use `DoubleCompare`/`DoubleEqual` to
handle floating-point tolerance. This can make CDF disagree with Rank/Quantile
around centroid boundaries due to rounding. Use `DoubleCompare` for the
comparisons here.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
+ std::string test_digest_name = "test_digest_cdf_nan" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> values = {0.0, 1.0, 2.0, 3.0};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, test_digest_name, values, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+ ASSERT_EQ(result.cdf_values.size(), values.size());
+ for (const auto cdf : result.cdf_values) {
+ EXPECT_TRUE(std::isnan(cdf));
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_duplicate_values) {
+ std::string test_digest_name = "test_cdf_duplicates" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {10, 10, 10, 20, 20});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {5, 10, 20, 25};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0, 0.3, 0.8, 1};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.001) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_signed_zero_queries) {
Review Comment:
Test case name `CDF_signed_zero_queries` is inconsistent with the
surrounding TDigest tests (which use PascalCase). Rename it to match the file’s
existing naming style.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
+ std::string test_digest_name = "test_digest_cdf_nan" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> values = {0.0, 1.0, 2.0, 3.0};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, test_digest_name, values, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+ ASSERT_EQ(result.cdf_values.size(), values.size());
+ for (const auto cdf : result.cdf_values) {
+ EXPECT_TRUE(std::isnan(cdf));
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_duplicate_values) {
+ std::string test_digest_name = "test_cdf_duplicates" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {10, 10, 10, 20, 20});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {5, 10, 20, 25};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0, 0.3, 0.8, 1};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.001) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_signed_zero_queries) {
+ std::string test_digest_name = "test_cdf_signed_zero" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {-1, 0, 1});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {-0.0, 0.0};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ EXPECT_NEAR(result.cdf_values[0], 0.5, 0.001);
+ EXPECT_NEAR(result.cdf_values[1], 0.5, 0.001);
+}
+
+TEST_F(RedisTDigestTest, CDF_uniform_distribution) {
+ std::string test_digest_name = "test_cdf_uniform" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {200}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = ranges::views::iota(1, 101) |
+ ranges::views::transform([](int i) { return
(double)i; }) |
+ ranges::to<std::vector<double>>();
+ status = tdigest_->Add(*ctx_, test_digest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {1, 25, 50, 75, 100};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.01, 0.25, 0.50, 0.75, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.02) <<
fmt::format("Mismatch at index {}, val={}", i, cdf_vals[i]);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_multiple_adds) {
Review Comment:
Test case name `CDF_multiple_adds` is inconsistent with the surrounding
TDigest tests (which use PascalCase). Rename it to match the file’s existing
naming style.
##########
src/types/redis_tdigest.cc:
##########
@@ -570,6 +573,109 @@ rocksdb::Status TDigest::Merge(engine::Context& ctx,
const Slice& dest_digest,
return storage_->Write(ctx, storage_->DefaultWriteOptions(),
batch->GetWriteBatch());
}
+rocksdb::Status TDigest::CDF(engine::Context& ctx, const Slice& digest_name,
const std::vector<double>& inputs,
+ TDigestCDFResult* result) {
+ std::map<double, std::vector<size_t>> sorted_unique_inputs_with_idx;
+ for (size_t i = 0; i < inputs.size(); ++i) {
+ sorted_unique_inputs_with_idx[inputs[i]].push_back(i);
+ }
+
+ std::vector<double> cdf_values;
+ if (auto status = cdfUniqSorted(ctx, digest_name,
sorted_unique_inputs_with_idx | ranges::views::keys | ranges::to_vector,
&cdf_values); !status.ok()) {
+ return status;
+ }
+ result->cdf_values.resize(inputs.size(),
std::numeric_limits<double>::quiet_NaN());
+
+ auto idx = 0;
+ for (auto iter = sorted_unique_inputs_with_idx.cbegin(); iter !=
sorted_unique_inputs_with_idx.cend(); ++iter) {
Review Comment:
`TDigest::CDF` currently uses a plain `std::map<double,...>` and `auto idx =
0`, which diverges from the tdigest numeric comparison/ordering approach used
elsewhere (e.g., Rank uses `DoubleComparator`). Using `DoubleComparator` avoids
surprising ordering/equality differences for close floating-point values and
makes the implementation consistent. Also make `idx` a `size_t` to avoid
signed/unsigned indexing warnings.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
Review Comment:
Test case name `CDF_Test` is inconsistent with the surrounding TDigest tests
(which use PascalCase without underscores). Rename it to match the file’s
existing naming style for easier filtering and consistency.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
Review Comment:
Test case name `CDF_returns_nan_on_empty_tdigest` is inconsistent with the
surrounding TDigest tests (which use PascalCase). Rename it to match the file’s
existing naming style.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
+ std::string test_digest_name = "test_digest_cdf_nan" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> values = {0.0, 1.0, 2.0, 3.0};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, test_digest_name, values, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+ ASSERT_EQ(result.cdf_values.size(), values.size());
+ for (const auto cdf : result.cdf_values) {
+ EXPECT_TRUE(std::isnan(cdf));
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_duplicate_values) {
Review Comment:
Test case name `CDF_duplicate_values` is inconsistent with the surrounding
TDigest tests (which use PascalCase). Rename it to match the file’s existing
naming style.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
+ std::string test_digest_name = "test_digest_cdf_nan" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> values = {0.0, 1.0, 2.0, 3.0};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, test_digest_name, values, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+ ASSERT_EQ(result.cdf_values.size(), values.size());
+ for (const auto cdf : result.cdf_values) {
+ EXPECT_TRUE(std::isnan(cdf));
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_duplicate_values) {
+ std::string test_digest_name = "test_cdf_duplicates" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {10, 10, 10, 20, 20});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {5, 10, 20, 25};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0, 0.3, 0.8, 1};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.001) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_signed_zero_queries) {
+ std::string test_digest_name = "test_cdf_signed_zero" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {-1, 0, 1});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {-0.0, 0.0};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ EXPECT_NEAR(result.cdf_values[0], 0.5, 0.001);
+ EXPECT_NEAR(result.cdf_values[1], 0.5, 0.001);
+}
+
+TEST_F(RedisTDigestTest, CDF_uniform_distribution) {
+ std::string test_digest_name = "test_cdf_uniform" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {200}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = ranges::views::iota(1, 101) |
+ ranges::views::transform([](int i) { return
(double)i; }) |
+ ranges::to<std::vector<double>>();
+ status = tdigest_->Add(*ctx_, test_digest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {1, 25, 50, 75, 100};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.01, 0.25, 0.50, 0.75, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.02) <<
fmt::format("Mismatch at index {}, val={}", i, cdf_vals[i]);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_multiple_adds) {
+ std::string test_digest_name = "test_cdf_multiadd" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples1 = {1, 2, 3, 4, 5};
+ std::vector<double> samples2 = {6, 7, 8, 9, 10};
+ status = tdigest_->Add(*ctx_, test_digest_name, samples1);
+ ASSERT_TRUE(status.ok());
+ status = tdigest_->Add(*ctx_, test_digest_name, samples2);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {1, 5, 7, 10};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> expected = {0.10, 0.50, 0.70, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR((result.cdf_values)[i], expected[i], 0.06)
+ << fmt::format("Mismatch at index {}, val={}", i, cdf_vals[i]);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_skewed_distribution) {
Review Comment:
Test case name `CDF_skewed_distribution` is inconsistent with the
surrounding TDigest tests (which use PascalCase). Rename it to match the file’s
existing naming style.
##########
tests/cppunit/types/tdigest_test.cc:
##########
@@ -948,3 +949,178 @@ TEST_F(RedisTDigestTest,
MergeWithUserSpecifiedCompression) {
// Verify total observations: dest(1) + src(1) = 2
EXPECT_EQ(metadata.total_observations, 2);
}
+
+TEST_F(RedisTDigestTest, CDF_Test) {
+ std::string cdf_tdigest_name = "test_cdf_digest" +
std::to_string(util::GetTimeStampMS());
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, cdf_tdigest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> samples = {1, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 5};
+ status = tdigest_->Add(*ctx_, cdf_tdigest_name, samples);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {0, 1, 2, 3, 4, 5, 6};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, cdf_tdigest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0.00, 0.03, 0.13, 0.29, 0.53, 0.83, 1.00};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.015) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_returns_nan_on_empty_tdigest) {
+ std::string test_digest_name = "test_digest_cdf_nan" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> values = {0.0, 1.0, 2.0, 3.0};
+ redis::TDigestCDFResult result;
+
+ status = tdigest_->CDF(*ctx_, test_digest_name, values, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+ ASSERT_EQ(result.cdf_values.size(), values.size());
+ for (const auto cdf : result.cdf_values) {
+ EXPECT_TRUE(std::isnan(cdf));
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_duplicate_values) {
+ std::string test_digest_name = "test_cdf_duplicates" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {10, 10, 10, 20, 20});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {5, 10, 20, 25};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ std::vector<double> expected = {0, 0.3, 0.8, 1};
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ for (size_t i = 0; i < cdf_vals.size(); i++) {
+ EXPECT_NEAR(result.cdf_values[i], expected[i], 0.001) <<
fmt::format("Mismatch at index {}", i);
+ }
+}
+
+TEST_F(RedisTDigestTest, CDF_signed_zero_queries) {
+ std::string test_digest_name = "test_cdf_signed_zero" +
std::to_string(util::GetTimeStampMS());
+
+ bool exists = false;
+ auto status = tdigest_->Create(*ctx_, test_digest_name, {100}, &exists);
+ ASSERT_FALSE(exists);
+ ASSERT_TRUE(status.ok());
+
+ status = tdigest_->Add(*ctx_, test_digest_name, {-1, 0, 1});
+ ASSERT_TRUE(status.ok());
+
+ std::vector<double> cdf_vals = {-0.0, 0.0};
+ redis::TDigestCDFResult result;
+ status = tdigest_->CDF(*ctx_, test_digest_name, cdf_vals, &result);
+ ASSERT_TRUE(status.ok()) << status.ToString();
+
+ ASSERT_EQ(result.cdf_values.size(), cdf_vals.size());
+ EXPECT_NEAR(result.cdf_values[0], 0.5, 0.001);
+ EXPECT_NEAR(result.cdf_values[1], 0.5, 0.001);
+}
+
+TEST_F(RedisTDigestTest, CDF_uniform_distribution) {
Review Comment:
Test case name `CDF_uniform_distribution` is inconsistent with the
surrounding TDigest tests (which use PascalCase). Rename it to match the file’s
existing naming style.
--
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]
