Skip to content

Commit

Permalink
ENG-2783 Fix bug in ZREM which causes it to create a redis hash type.
Browse files Browse the repository at this point in the history
Summary:
If the number of keys to delete in ZREM is 0, it inadvertently creates a redis hash data
type due to the SubDocument being empty. As a result, if we try a ZADD on that same set later on, it
returns a wrong type error.

The fix in this case was to not attempt the deletion altogether if the number of keys is 0.

Test Plan: unit test.

Reviewers: rahuldesirazu, amitanand, kannan, akashnil

Reviewed By: amitanand, kannan, akashnil

Subscribers: bogdan, ram, kannan, ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D4011
  • Loading branch information
pritamdamania87 committed Jan 29, 2018
1 parent 9466536 commit a3a4401
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 3 deletions.
10 changes: 7 additions & 3 deletions src/yb/docdb/doc_operation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,7 @@ Status RedisWriteOperation::ApplyDel(const DocOperationApplyData& data) {
RETURN_NOT_OK(PrimitiveValueFromSubKeyStrict(kv.subkey(i), *data_type, &primitive_value));
values.SetChild(primitive_value, SubDocument(ValueType::kTombstone));
}
num_keys = kv.subkey_size();
break;
}
case REDIS_TYPE_SORTEDSET: {
Expand Down Expand Up @@ -839,9 +840,12 @@ Status RedisWriteOperation::ApplyDel(const DocOperationApplyData& data) {
break;
}
}
DocPath doc_path = DocPath::DocPathFromRedisKey(kv.hash_code(), kv.key());
RETURN_NOT_OK(data.doc_write_batch->ExtendSubDocument(
doc_path, values, redis_query_id()));

if (num_keys != 0) {
DocPath doc_path = DocPath::DocPathFromRedisKey(kv.hash_code(), kv.key());
RETURN_NOT_OK(data.doc_write_batch->ExtendSubDocument(doc_path, values, redis_query_id()));
}

response_.set_code(RedisResponsePB_RedisStatusCode_OK);
if (EmulateRedisResponse(kv.type())) {
// If the flag is true, we respond with the number of keys actually being deleted. We don't
Expand Down
11 changes: 11 additions & 0 deletions src/yb/yql/redis/redisserver/redisserver-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1339,6 +1339,17 @@ TEST_F(TestRedisService, TestSortedSets) {
// Cannot have incr with multiple score value pairs.
DoRedisTestExpectError(__LINE__, {"ZADD", "z_key", "INCR", "0", "v1", "1", "v2"});

// Test ZREM on non-existent key and then add the same key.
DoRedisTestInt(__LINE__, {"ZREM", "my_z_set", "v1"}, 0);
SyncClient();
DoRedisTestInt(__LINE__, {"ZCARD", "my_z_set"}, 0);
SyncClient();
DoRedisTestInt(__LINE__, {"ZADD", "my_z_set", "1", "v1"}, 1);
SyncClient();
DoRedisTestInt(__LINE__, {"ZCARD", "my_z_set"}, 1);
SyncClient();
DoRedisTestArray(__LINE__, {"ZRANGEBYSCORE", "my_z_set", "1", "1"}, {"v1"});

SyncClient();
VerifyCallbacks();
}
Expand Down

0 comments on commit a3a4401

Please sign in to comment.