Skip to content

Commit 27fbff1

Browse files
Tangruilinjihuayu
andauthored
fix(tdigest): correct TDIGEST.MERGE parser for COMPRESSION parameter. (#3449)
Fix #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. Signed-off-by: reilly <reilly@example.com> Co-authored-by: 纪华裕 <jihuayu123@gmail.com>
1 parent 505108c commit 27fbff1

2 files changed

Lines changed: 48 additions & 3 deletions

File tree

src/commands/cmd_tdigest.cc

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -466,9 +466,7 @@ class CommandTDigestMerge : public Commander {
466466
} else {
467467
options_.compression = *compression;
468468
}
469-
}
470-
471-
if (parser.EatEqICase(kOverrideArg)) {
469+
} else if (parser.EatEqICase(kOverrideArg)) {
472470
if (options_.override_flag) { // override already set
473471
return {Status::RedisParseErr, errWrongNumOfArguments};
474472
}

tests/gocase/unit/type/tdigest/tdigest_test.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -642,6 +642,53 @@ func tdigestTests(t *testing.T, configs util.KvrocksServerConfigs) {
642642
validation(newDestKey2)
643643
})
644644

645+
// https://github.com/apache/kvrocks/issues/3447
646+
t.Run("tdigest.merge with COMPRESSION parameter (issue #3447)", func(t *testing.T) {
647+
keyPrefix := "tdigest_merge_compression_"
648+
649+
srcKey := keyPrefix + "src"
650+
destKey := keyPrefix + "dest"
651+
652+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", srcKey, "compression", "100").Err())
653+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", srcKey, "1", "2", "3", "4", "5").Err())
654+
655+
// This should succeed, not return "ERR wrong keyword"
656+
// Issue #3447: TDIGEST.MERGE dest 1 src COMPRESSION 100 fails with "ERR wrong keyword"
657+
require.NoError(t, rdb.Do(ctx, "TDIGEST.MERGE", destKey, 1, srcKey, "COMPRESSION", "100").Err())
658+
659+
// Verify the merge worked
660+
rsp := rdb.Do(ctx, "TDIGEST.INFO", destKey)
661+
require.NoError(t, rsp.Err())
662+
info := toTdigestInfo(t, rsp.Val())
663+
require.EqualValues(t, 100, info.Compression)
664+
require.EqualValues(t, 5, info.Observations)
665+
})
666+
667+
// Test COMPRESSION + OVERRIDE combination
668+
t.Run("tdigest.merge with COMPRESSION and OVERRIDE together", func(t *testing.T) {
669+
keyPrefix := "tdigest_merge_compression_override_"
670+
671+
srcKey := keyPrefix + "src"
672+
destKey := keyPrefix + "dest"
673+
674+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", srcKey, "compression", "100").Err())
675+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", srcKey, "1", "2", "3", "4", "5").Err())
676+
677+
// Create destination key first to test OVERRIDE
678+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", destKey, "compression", "50").Err())
679+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", destKey, "10", "20").Err())
680+
681+
// COMPRESSION + OVERRIDE should work together
682+
require.NoError(t, rdb.Do(ctx, "TDIGEST.MERGE", destKey, 1, srcKey, "COMPRESSION", "101", "OVERRIDE").Err())
683+
684+
// Verify the merge worked with new compression
685+
rsp := rdb.Do(ctx, "TDIGEST.INFO", destKey)
686+
require.NoError(t, rsp.Err())
687+
info := toTdigestInfo(t, rsp.Val())
688+
require.EqualValues(t, 101, info.Compression)
689+
require.EqualValues(t, 5, info.Observations) // src data replaced dest data due to OVERRIDE
690+
})
691+
645692
t.Run("tdigest.revrank with different arguments", func(t *testing.T) {
646693
keyPrefix := "tdigest_revrank_"
647694

0 commit comments

Comments
 (0)