Copilot commented on code in PR #3312:
URL: https://github.com/apache/kvrocks/pull/3312#discussion_r2905717890
##########
src/commands/cmd_tdigest.cc:
##########
@@ -492,6 +492,67 @@ class CommandTDigestMerge : public Commander {
TDigestMergeOptions options_;
};
+class CommandTDigestTrimmedMean : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ if (args.size() != 4) {
+ return {Status::RedisParseErr, errWrongNumOfArguments};
+ }
+
+ key_name_ = args[1];
+
+ auto low_cut_quantile = ParseFloat(args[2]);
+ if (!low_cut_quantile) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ low_cut_quantile_ = *low_cut_quantile;
+
+ auto high_cut_quantile = ParseFloat(args[3]);
+ if (!high_cut_quantile) {
+ return {Status::RedisParseErr, errValueIsNotFloat};
+ }
+ high_cut_quantile_ = *high_cut_quantile;
+
+ if (low_cut_quantile_ < 0.0 || low_cut_quantile_ > 1.0) {
+ return {Status::RedisParseErr, errLowCutQuantileRange};
+ }
+ if (high_cut_quantile_ < 0.0 || high_cut_quantile_ > 1.0) {
+ return {Status::RedisParseErr, errHighCutQuantileRange};
+ }
+ if (DoubleCompare(low_cut_quantile_, high_cut_quantile_) >= 0) {
+ return {Status::RedisParseErr, errLowCutQuantileLess};
+ }
Review Comment:
The quantile validation doesn’t handle NaN: `ParseFloat("nan")` succeeds,
but comparisons like `< 0.0` / `> 1.0` will be false, and `DoubleCompare` with
NaN will route to the "low cut quantile must be less..." error (or even allow
invalid values through in other cases). Explicitly reject NaN (and ideally
non-finite values) for both `low_cut_quantile_` and `high_cut_quantile_` so
invalid inputs consistently return the intended range errors.
##########
src/types/redis_tdigest.cc:
##########
@@ -759,6 +759,41 @@ rocksdb::Status
TDigest::applyNewCentroids(ObserverOrUniquePtr<rocksdb::WriteBat
return rocksdb::Status::OK();
}
+rocksdb::Status TDigest::TrimmedMean(engine::Context& ctx, const Slice&
digest_name, double low_cut_quantile,
+ double high_cut_quantile,
TDigestTrimmedMeanResult* result) {
+ 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) {
Review Comment:
`TrimmedMean` returns early when `metadata.total_observations == 0` without
clearing `result->mean`. If callers reuse the same `TDigestTrimmedMeanResult`
instance across calls, this can leave a stale mean from a previous non-empty
digest. Set `result->mean.reset()` (or assign `std::nullopt`) before returning
OK for the empty-digest path.
```suggestion
if (metadata.total_observations == 0) {
result->mean.reset();
```
--
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]
