You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "Tangruilin (via GitHub)" <gi...@apache.org> on 2026/04/13 14:31:39 UTC
[PR] fix(tdigest): correct TDIGEST.MERGE parser for COMPRESSION parameter … [kvrocks]
Tangruilin opened a new pull request, #3449:
URL: https://github.com/apache/kvrocks/pull/3449
…(#3447)
The parser incorrectly used two independent `if` statements to check COMPRESSION and OVERRIDE parameters. When COMPRESSION was successfully parsed, the code continued to the second `if` which would fail since the parser had reached the end, causing "ERR wrong keyword" error.
Fixed by changing the second `if` to `else if` to form a proper if-else-if-else chain.
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "LindaSummer (via GitHub)" <gi...@apache.org>.
LindaSummer merged PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter … [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3077729034
##########
.gitignore:
##########
@@ -52,3 +52,6 @@
/build/
/cmake-build-*/
/build-*/
+
+# go cache
+/pkg
Review Comment:
Why change it?
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "LindaSummer (via GitHub)" <gi...@apache.org>.
LindaSummer commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3080819715
##########
src/commands/cmd_tdigest.cc:
##########
@@ -466,9 +466,7 @@ class CommandTDigestMerge : public Commander {
} else {
options_.compression = *compression;
}
- }
-
- if (parser.EatEqICase(kOverrideArg)) {
+ } else if (parser.EatEqICase(kOverrideArg)) {
Review Comment:
Refer to [Redis official doc tdiget.merge](https://redis.io/docs/latest/commands/tdigest.merge/), the `override` is optional after `compression`.
> TDIGEST.MERGE destination-key numkeys source-key [source-key ...] [COMPRESSION compression] [OVERRIDE]
This change would make `tdigest.merge dest_key 1 source_key compression 101 override` discard the `override` option and lead to unexpected behavior.
##########
.gitignore:
##########
@@ -52,3 +52,6 @@
/build/
/cmake-build-*/
/build-*/
+
+# go cache
+/pkg
Review Comment:
We should keep SRP (Single Responsibility Principle) for each patch.
It is very important when we try to publish release and cherry-pick patches to specific version.
If we need this gitignore change, please create a new PR for it.
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#issuecomment-4249795525
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [66.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_coverage&view=list)
 [1.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449)
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#issuecomment-4248527047
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [67.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_coverage&view=list)
 [1.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449)
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3080994041
##########
.gitignore:
##########
@@ -52,3 +52,6 @@
/build/
/cmake-build-*/
/build-*/
+
+# go cache
+/pkg
Review Comment:
okey. i will submit it in another patch
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter … [kvrocks]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3079077753
##########
.gitignore:
##########
@@ -52,3 +52,6 @@
/build/
/cmake-build-*/
/build-*/
+
+# go cache
+/pkg
Review Comment:
The /pkg directory is where Go caches dependencies when running tests. If not ignored, you'd have to be careful every time you submit a PR to avoid accidentally committing it.
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3081051988
##########
src/commands/cmd_tdigest.cc:
##########
@@ -466,9 +466,7 @@ class CommandTDigestMerge : public Commander {
} else {
options_.compression = *compression;
}
- }
-
- if (parser.EatEqICase(kOverrideArg)) {
+ } else if (parser.EatEqICase(kOverrideArg)) {
Review Comment:
Actually, this won't cause any issue. The implementation in kvrocks differs from redisBloom here - kvrocks ensures that the trailing override won't be ignored through `while(parser.Good())`. I
had already considered this scenario before making the code changes.
PS: I've added corresponding test cases 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): correct TDIGEST.MERGE parser for COMPRESSION parameter … [kvrocks]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#issuecomment-4237184075
fix before:
<img width="1452" height="504" alt="image" src="https://github.com/user-attachments/assets/95c1c1ae-896d-4a6a-96dc-d7412e16efc1" />
after:
<img width="1387" height="742" alt="image" src="https://github.com/user-attachments/assets/bf731388-cfb0-45b7-8005-00fb83a8e208" />
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter … [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#issuecomment-4241024524
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3449&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=3449&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [69.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3449&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3449)
--
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): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]
Posted by "LindaSummer (via GitHub)" <gi...@apache.org>.
LindaSummer commented on code in PR #3449:
URL: https://github.com/apache/kvrocks/pull/3449#discussion_r3084274365
##########
src/commands/cmd_tdigest.cc:
##########
@@ -466,9 +466,7 @@ class CommandTDigestMerge : public Commander {
} else {
options_.compression = *compression;
}
- }
-
- if (parser.EatEqICase(kOverrideArg)) {
+ } else if (parser.EatEqICase(kOverrideArg)) {
Review Comment:
Sorry for my misunderstanding of this part. You are right, and the refactor should work well. 👍
--
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