Skip to content

Commit

Permalink
ENG-3164. Removed exclusive index bounds in ZRevRange.
Browse files Browse the repository at this point in the history
Summary: Removed exclusive bounds ( i.e. (4 ) from ZRevRange so passing this is now returns an error, which matches the redis cluster behavior.

Test Plan: c++ unit tests.

Reviewers: amitanand, hector, pritam.damania

Reviewed By: pritam.damania

Differential Revision: https://phabricator.dev.yugabyte.com/D4566
  • Loading branch information
rahuldesirazu committed Apr 12, 2018
1 parent 2603764 commit a7d1447
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 27 deletions.
1 change: 0 additions & 1 deletion src/yb/common/redis_protocol.proto
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ message RedisIndexRangePB {

message RedisIndexBoundPB {
required int64 index = 1;
optional bool is_exclusive = 2 [default = false];
}

// Wrapper for a subkey which denotes an upper/lower bound for a range request.
Expand Down
8 changes: 2 additions & 6 deletions src/yb/docdb/doc_operation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1324,13 +1324,9 @@ Status RedisReadOperation::ExecuteCollectionGetRange() {
PrimitiveValue(ValueType::kSSForward).AppendToKey(&encoded_doc_key);

bool add_keys = request_.get_collection_range_request().with_scores();
bool low_is_exclusive = low_index_bound.is_exclusive();
bool high_is_exclusive = high_index_bound.is_exclusive();

IndexBound low_bound =
IndexBound(low_idx_normalized, low_is_exclusive, true /* is_lower */);
IndexBound high_bound =
IndexBound(high_idx_normalized, high_is_exclusive, false /* is_lower */);
IndexBound low_bound = IndexBound(low_idx_normalized, true /* is_lower */);
IndexBound high_bound = IndexBound(high_idx_normalized, false /* is_lower */);

SubDocument doc;
bool doc_found = false;
Expand Down
11 changes: 2 additions & 9 deletions src/yb/docdb/docdb.h
Original file line number Diff line number Diff line change
Expand Up @@ -296,30 +296,23 @@ class IndexBound {
public:
IndexBound() :
index_(-1),
is_exclusive_(false),
is_lower_bound_(false) {}

IndexBound(int64 index, bool is_exclusive, bool is_lower_bound) :
IndexBound(int64 index, bool is_lower_bound) :
index_(index),
is_exclusive_(is_exclusive),
is_lower_bound_(is_lower_bound) {}

bool CanInclude(int64 curr_index) const {
if (index_ == -1 ) {
return true;
}
if (is_lower_bound_) {
return (is_exclusive_) ? (index_ < curr_index) : (index_ <= curr_index);
} else {
return (is_exclusive_) ? (index_ > curr_index) : (index_ >= curr_index);
}
return is_lower_bound_ ? index_ <= curr_index : index_ >= curr_index;
}

static const IndexBound& Empty();

private:
const int64 index_;
const bool is_exclusive_;
const bool is_lower_bound_;
};

Expand Down
12 changes: 2 additions & 10 deletions src/yb/yql/redis/redisserver/redis_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -505,11 +505,10 @@ CHECKED_STATUS ParseTsBoundArg(const Slice& slice, RedisSubKeyBoundPB* bound_pb,
}

CHECKED_STATUS
ParseIndexBoundArg(const Slice& slice, RedisIndexBoundPB* bound_pb, bool exclusive) {
ParseIndexBoundArg(const Slice& slice, RedisIndexBoundPB* bound_pb) {
auto index_bound = util::CheckedStoll(slice);
RETURN_NOT_OK(index_bound);
bound_pb->set_index(*index_bound);
bound_pb->set_is_exclusive(exclusive);
return Status::OK();
}

Expand All @@ -534,14 +533,7 @@ CHECKED_STATUS ParseIndexBound(const Slice& slice, RedisIndexBoundPB* bound_pb)
if (slice.empty()) {
return STATUS(InvalidArgument, "range bound index cannot be empty");
}

if (slice[0] == '(' && slice.size() > 1) {
auto slice_copy = slice;
slice_copy.remove_prefix(1);
RETURN_NOT_OK(ParseIndexBoundArg(slice_copy, bound_pb, /* exclusive */ true));
} else {
RETURN_NOT_OK(ParseIndexBoundArg(slice, bound_pb, /* exclusive */ false));
}
RETURN_NOT_OK(ParseIndexBoundArg(slice, bound_pb));
return Status::OK();
}

Expand Down
4 changes: 3 additions & 1 deletion src/yb/yql/redis/redisserver/redisserver-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1633,7 +1633,6 @@ TEST_F(TestRedisService, TestZRevRange) {
{1, 1, 1, 0, 0, 0},
{"v5", "v4", "v3", "v2", "v1", "v0"});
DoRedisTestArray(__LINE__, {"ZREVRANGE", "z_multi", "0", "1"}, {"v5", "v4"});
DoRedisTestArray(__LINE__, {"ZREVRANGE", "z_multi", "(0", "(1"}, {});
DoRedisTestArray(__LINE__, {"ZREVRANGE", "z_multi", "2", "3"}, {"v3", "v2"});
DoRedisTestArray(__LINE__, {"ZREVRANGE", "z_multi", "6", "7"}, {});
DoRedisTestArray(__LINE__, {"ZREVRANGE", "z_multi", "0", "-1"},
Expand All @@ -1650,6 +1649,9 @@ TEST_F(TestRedisService, TestZRevRange) {
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "z_multi", "1", "2", "WITHSCORES", "1"});
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "z_multi", "1.0", "2.0"});
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "1", "2"});
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "z_multi", "0", "(2"});
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "z_multi", "(0", "2"});
DoRedisTestExpectError(__LINE__, {"ZREVRANGE", "z_multi", "(0", "(2"});

// Test key with wrong type.
DoRedisTestOk(__LINE__, {"SET", "s_key", "s_val"});
Expand Down

0 comments on commit a7d1447

Please sign in to comment.