You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "LindaSummer (via GitHub)" <gi...@apache.org> on 2025/04/13 10:42:08 UTC
[PR] bugfix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
LindaSummer opened a new pull request, #2878:
URL: https://github.com/apache/kvrocks/pull/2878
## Issue
TDigest only cares about weight when merging, so it is possible to have centroids with same mean.
This will lead the centroids overwriting during serialization.
## Proposed Changes
Add a seq id to make centroids mean encoding keep same order as the original centroids list without confliction.
## Related Issue
#2849
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#issuecomment-2806951232
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878) **Quality Gate failed**
Failed conditions
 [2 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#issuecomment-2801031545
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878) **Quality Gate failed**
Failed conditions
 [2 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "LindaSummer (via GitHub)" <gi...@apache.org>.
LindaSummer commented on code in PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#discussion_r2045841999
##########
src/types/redis_tdigest.cc:
##########
@@ -332,10 +332,13 @@ std::string TDigest::internalBufferKey(const std::string& ns_key, const TDigestM
}
std::string TDigest::internalKeyFromCentroid(const std::string& ns_key, const TDigestMetadata& metadata,
- const Centroid& centroid) const {
+ const Centroid& centroid, uint32_t seq) const {
std::string sub_key;
PutFixed8(&sub_key, static_cast<uint8_t>(SegmentType::kCentroids));
PutDouble(&sub_key, centroid.mean); // It uses EncodeDoubleToUInt64 and keeps original order of double
+ // The tdigest centroids only cares about the weight rather than the mean, so different centroids may have same mean,
+ // we should keep them with same original order, this seq id could be discarded in decode
+ PutFixed32(&sub_key, seq);
Review Comment:
Hi @mapleFU ,
Thanks very much for your kind review!😊
I thought about the size when adding this seq-id.
The compression is uint32 now, so the limitation of centroids is uint32 maximum.
So I decided to use uint32 for safety.
Best Regards,
Edward
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#discussion_r2044632749
##########
src/types/redis_tdigest.cc:
##########
@@ -332,10 +332,13 @@ std::string TDigest::internalBufferKey(const std::string& ns_key, const TDigestM
}
std::string TDigest::internalKeyFromCentroid(const std::string& ns_key, const TDigestMetadata& metadata,
- const Centroid& centroid) const {
+ const Centroid& centroid, uint32_t seq) const {
std::string sub_key;
PutFixed8(&sub_key, static_cast<uint8_t>(SegmentType::kCentroids));
PutDouble(&sub_key, centroid.mean); // It uses EncodeDoubleToUInt64 and keeps original order of double
+ // The tdigest centroids only cares about the weight rather than the mean, so different centroids may have same mean,
+ // we should keep them with same original order, this seq id could be discarded in decode
+ PutFixed32(&sub_key, seq);
Review Comment:
Is Fixed16 enough for this?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix centroid encoding overwrite for same mean [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#issuecomment-2804171655
Sorry for delaying, I'm quite busy this week, will take a round tonight
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tdigest): fix tdigest centroid encoding overwrite for same mean [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2878:
URL: https://github.com/apache/kvrocks/pull/2878#issuecomment-2800402705
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878) **Quality Gate failed**
Failed conditions
 [2 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2878)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org