mapleFU commented on code in PR #2811: URL: https://github.com/apache/kvrocks/pull/2811#discussion_r1977545501
########## src/types/redis_tdigest.cc: ########## @@ -161,6 +161,8 @@ rocksdb::Status TDigest::Add(engine::Context& ctx, const Slice& digest_name, con metadata.total_observations += inputs.size(); metadata.total_weight += inputs.size(); + metadata.maximum = std::max(metadata.maximum, *std::max_element(inputs.cbegin(), inputs.cend())); Review Comment: To change the word here, what's the main purpose to set min max after merge🤔? Since update min max is free when calling Add if any io really happens. And `Quantile` will always merge before doing any real things? Without updating min max in add, the `tdigest.min/max` will traverse the unmerged data buffer Both way is ok to me here but I just don't know the point or trade-off here -- 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]
