feat(server): Implement BITPOS. #907
Conversation
Fixes dragonflydb#835. Signed-off-by: chakaz <chakaz@chakaz>
src/server/bitops_family.cc
Outdated
|
|
||
| if (position == -1 && !bit_value && static_cast<size_t>(start) < result_value.size() && | ||
| end == std::numeric_limits<int64_t>::max()) { | ||
| // Not-found return value is compatible with Redis, but is subtle. |
There was a problem hiding this comment.
The comment is a bit enigmatic to me :)
If returning -1 is compatible with Redis I am not sure I agree with the logic, or with the cognitive burden on the reader this edge-case yields.
There was a problem hiding this comment.
In this specific case, Redis returns (and documents that it returns) the size, in bits, of the value.
I agree, this edge case is super weird and has a potentially significant burden on users. But that's the Redis API, and Dragonfly aims to be compatible, so we must mimic this weird API :(
I slightly modified the comment, please lmk if that's not good enough or if you have other suggestions.
src/server/bitops_family.cc
Outdated
|
|
||
| OpResult<int64_t> FindFirstBitWithValue(const OpArgs& op_args, std::string_view key, bool bit_value, | ||
| int64_t start, int64_t end, bool as_bit) { | ||
| OpResult<std::string> result = ReadValue(op_args.db_cntx, key, op_args.shard); |
There was a problem hiding this comment.
rename result to value?
Result is a bit confusing with the result of the function.
There was a problem hiding this comment.
I wanted to distinguish bit_value from value, but you're right that it's more confusing as is. Renamed to value per your suggestion.
src/server/bitops_family.cc
Outdated
| int64_t start, int64_t end, bool as_bit) { | ||
| OpResult<std::string> result = ReadValue(op_args.db_cntx, key, op_args.shard); | ||
|
|
||
| std::string_view result_value; |
There was a problem hiding this comment.
Same here, could be input_value or similar.
There was a problem hiding this comment.
I went with value_str to reflect that it's the value's string value. lmk if you have a better idea.
src/server/bitops_family_test.cc
Outdated
| EXPECT_EQ(35, CheckedInt({"bitpos", "a", "1", "-5", "-1", "BIT"})); | ||
| EXPECT_EQ(34, CheckedInt({"bitpos", "a", "1", "-6", "-1", "BIT"})); | ||
|
|
||
| // Test weird return value spec, specifically when looking for clear bits in |
There was a problem hiding this comment.
// Edge cases for for clear bits in an all-set string.
There was a problem hiding this comment.
It's actually not the edge case that I'm testing, but rather that we behave like Redis in terms of the weird spec.
I updated the comment.
Signed-off-by: chakaz <chakaz@chakaz>
Signed-off-by: chakaz <chakaz@chakaz>
src/server/bitops_family_test.cc
Outdated
| EXPECT_EQ(-1, CheckedInt({"bitpos", "b", "0", "0", "3"})); | ||
| EXPECT_EQ(-1, CheckedInt({"bitpos", "b", "0", "0", "3", "BYTE"})); | ||
|
|
||
| ASSERT_EQ(Run({"set", "c", ""_b}), "OK"); |
There was a problem hiding this comment.
Nit: I'd call this key "empty"
Signed-off-by: chakaz <chakaz@chakaz>
Fixes #835.