Re: [PR] fix(tdigest): correct TDIGEST.MERGE parser for COMPRESSION parameter. [kvrocks]

["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