Skip to content

Commit 385805a

Browse files
committed
fix(tdigest): correct TDIGEST.MERGE parser for COMPRESSION parameter (#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>
1 parent 94e4c8e commit 385805a

File tree

2 files changed

+48
-3
lines changed

2 files changed

+48
-3
lines changed

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
@@ -525,6 +525,53 @@ func tdigestTests(t *testing.T, configs util.KvrocksServerConfigs) {
525525
validation(newDestKey2)
526526
})
527527

528+
// https://github.com/apache/kvrocks/issues/3447
529+
t.Run("tdigest.merge with COMPRESSION parameter (issue #3447)", func(t *testing.T) {
530+
keyPrefix := "tdigest_merge_compression_"
531+
532+
srcKey := keyPrefix + "src"
533+
destKey := keyPrefix + "dest"
534+
535+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", srcKey, "compression", "100").Err())
536+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", srcKey, "1", "2", "3", "4", "5").Err())
537+
538+
// This should succeed, not return "ERR wrong keyword"
539+
// Issue #3447: TDIGEST.MERGE dest 1 src COMPRESSION 100 fails with "ERR wrong keyword"
540+
require.NoError(t, rdb.Do(ctx, "TDIGEST.MERGE", destKey, 1, srcKey, "COMPRESSION", "100").Err())
541+
542+
// Verify the merge worked
543+
rsp := rdb.Do(ctx, "TDIGEST.INFO", destKey)
544+
require.NoError(t, rsp.Err())
545+
info := toTdigestInfo(t, rsp.Val())
546+
require.EqualValues(t, 100, info.Compression)
547+
require.EqualValues(t, 5, info.Observations)
548+
})
549+
550+
// Test COMPRESSION + OVERRIDE combination
551+
t.Run("tdigest.merge with COMPRESSION and OVERRIDE together", func(t *testing.T) {
552+
keyPrefix := "tdigest_merge_compression_override_"
553+
554+
srcKey := keyPrefix + "src"
555+
destKey := keyPrefix + "dest"
556+
557+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", srcKey, "compression", "100").Err())
558+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", srcKey, "1", "2", "3", "4", "5").Err())
559+
560+
// Create destination key first to test OVERRIDE
561+
require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", destKey, "compression", "50").Err())
562+
require.NoError(t, rdb.Do(ctx, "TDIGEST.ADD", destKey, "10", "20").Err())
563+
564+
// COMPRESSION + OVERRIDE should work together
565+
require.NoError(t, rdb.Do(ctx, "TDIGEST.MERGE", destKey, 1, srcKey, "COMPRESSION", "101", "OVERRIDE").Err())
566+
567+
// Verify the merge worked with new compression
568+
rsp := rdb.Do(ctx, "TDIGEST.INFO", destKey)
569+
require.NoError(t, rsp.Err())
570+
info := toTdigestInfo(t, rsp.Val())
571+
require.EqualValues(t, 101, info.Compression)
572+
require.EqualValues(t, 5, info.Observations) // src data replaced dest data due to OVERRIDE
573+
})
574+
528575
t.Run("tdigest.revrank with different arguments", func(t *testing.T) {
529576
keyPrefix := "tdigest_revrank_"
530577

0 commit comments

Comments
 (0)