You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "infdahai (via GitHub)" <gi...@apache.org> on 2023/06/11 09:06:38 UTC
[GitHub] [incubator-kvrocks] infdahai opened a new pull request, #1492: Fix zset error: return nilstring when limit cnt == 0
infdahai opened a new pull request, #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492
closes #1483
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1492: Fix zset error: return nilstring when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1586681496
```sh
ok github.com/apache/incubator-kvrocks/tests/gocase/unit/type/strings 45.094s
--- FAIL: TestZset (6.78s)
--- FAIL: TestZset/ZRANGE_basics_-_skiplist (0.00s)
zset_test.go:445:
Error Trace: /Users/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/type/zset/zset_test.go:445
Error: Not equal:
expected: []string{}
actual : []string(nil)
Diff:
--- Expected
+++ Actual
@@ -1,3 +1,2 @@
-([]string) {
-}
+([]string) <nil>
Test: TestZset/ZRANGE_basics_-_skiplist
--- FAIL: TestZset/ZRANGEBYLEX_with_LIMIT (0.00s)
zset_test.go:778:
Error Trace: /Users/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/type/zset/zset_test.go:778
Error: Not equal:
expected: []interface {}([]interface {}{})
actual : <nil>(<nil>)
Test: TestZset/ZRANGEBYLEX_with_LIMIT
FAIL
```
Hi, if I return the nil string in the redis server, how can I match this msg in go-redis?
just Use Errorf("redis: nil")?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1492: Fix ZRANGE command should return an empty array when count = 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1592276834
Thanks all, merging..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1227982355
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -439,6 +440,23 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
{4, "d"},
}, rdb.ZRangeWithScores(ctx, "ztmp", 0, -1).Val())
+ for i := 0; i < 10; i++ {
+ cmd := rdb.ZRangeArgs(ctx, redis.ZRangeArgs{Key: "ztmp", Count: 0, ByScore: true, Start: 0, Stop: -1, Offset: int64(i)})
+ require.Equal(t, []string{}, cmd.Val())
Review Comment:
Thanks. I didn't read the process in go-redis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1228516547
##########
src/commands/cmd_zset.cc:
##########
@@ -555,9 +561,17 @@ class CommandZRangeGeneric : public Commander {
s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
break;
case kZRangeScore:
+ if (score_spec_.count == 0) {
+ *output = redis::EmptyArray();
+ return Status::OK();
+ }
s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
break;
case kZRangeLex:
+ if (lex_spec_.count == 0) {
+ *output = redis::EmptyArray();
Review Comment:
we can use `std::vector<std::string> values; *output =MultiBulkString(values, false)` or `*output=redis::MultiLen(0)` to do the same thing.
I'm not sure if I want to add the specific `EmptyArray`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1227800004
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -439,6 +440,23 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
{4, "d"},
}, rdb.ZRangeWithScores(ctx, "ztmp", 0, -1).Val())
+ for i := 0; i < 10; i++ {
+ cmd := rdb.ZRangeArgs(ctx, redis.ZRangeArgs{Key: "ztmp", Count: 0, ByScore: true, Start: 0, Stop: -1, Offset: int64(i)})
+ require.Equal(t, []string{}, cmd.Val())
Review Comment:
https://github.com/redis/go-redis/blame/0bdc7dd8986e4923b3309f880245b9311d09f273/commands.go#L2790 won't append the `LIMIT offset,count` if the offset and counter are zero. So for the kvrocks side, it will regard it as the default value which is offset = 0 and count = -1.
By the way, we should check the error first before comparing the value. Same as the following lines, we need to prevent the limit and counter are 0 at the same time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1492: Fix zset error: return nilstring when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1586195252
Can I ask if we use nilString and we use what to match the value or err ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1228882393
##########
src/commands/cmd_zset.cc:
##########
@@ -555,9 +561,17 @@ class CommandZRangeGeneric : public Commander {
s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
break;
case kZRangeScore:
+ if (score_spec_.count == 0) {
+ *output = redis::EmptyArray();
+ return Status::OK();
+ }
s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
break;
case kZRangeLex:
+ if (lex_spec_.count == 0) {
+ *output = redis::EmptyArray();
Review Comment:
Yes, can remove it. Others are good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1492: Fix ZRANGE command should return an empty array when count = 0
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1229338461
##########
src/commands/cmd_zset.cc:
##########
@@ -555,9 +561,17 @@ class CommandZRangeGeneric : public Commander {
s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
break;
case kZRangeScore:
+ if (score_spec_.count == 0) {
+ *output = redis::EmptyArray();
+ return Status::OK();
+ }
s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
break;
case kZRangeLex:
+ if (lex_spec_.count == 0) {
+ *output = redis::EmptyArray();
Review Comment:
The third option is just `*output = MultiBulkString({});` since the array is empty and there is no need to care whether use an empty string or nil.
Other changes are good to me as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai closed pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai closed pull request #1492: Fix zset error: return the empty array when limit cnt == 0
URL: https://github.com/apache/incubator-kvrocks/pull/1492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1227754031
##########
src/commands/cmd_zset.cc:
##########
@@ -513,6 +520,8 @@ class CommandZRangeGeneric : public Commander {
max_idx = 2;
}
+ if (count == 0) zero_count_ = true;
Review Comment:
I think it'd be better to use `spec.count == 0` to check if the count is zero instead of adding a new variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1492: Fix zset error: return nilstring when limit cnt == 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1586693958
@infdahai You can use expression like `require.EqualError(t, r.Err(), redis.Nil.Error())`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1492: Fix ZRANGE command should return an empty array when count = 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1227800004
##########
tests/gocase/unit/type/zset/zset_test.go:
##########
@@ -439,6 +440,23 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, encoding s
{4, "d"},
}, rdb.ZRangeWithScores(ctx, "ztmp", 0, -1).Val())
+ for i := 0; i < 10; i++ {
+ cmd := rdb.ZRangeArgs(ctx, redis.ZRangeArgs{Key: "ztmp", Count: 0, ByScore: true, Start: 0, Stop: -1, Offset: int64(i)})
+ require.Equal(t, []string{}, cmd.Val())
Review Comment:
https://github.com/redis/go-redis/blame/0bdc7dd8986e4923b3309f880245b9311d09f273/commands.go#L2790 won't append the `LIMIT offset,count` if the offset and counter are zero. So for the kvrocks side, it will regard it as the default value which is offset = 0 and count = -1.
By the way, we should check the error first before comparing the value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1228516547
##########
src/commands/cmd_zset.cc:
##########
@@ -555,9 +561,17 @@ class CommandZRangeGeneric : public Commander {
s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
break;
case kZRangeScore:
+ if (score_spec_.count == 0) {
+ *output = redis::EmptyArray();
+ return Status::OK();
+ }
s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
break;
case kZRangeLex:
+ if (lex_spec_.count == 0) {
+ *output = redis::EmptyArray();
Review Comment:
we can use `std::vector<std::string> values; *output =MultiBulkString(values, false)` to do the same thing.
I'm not sure if I want to add the specific `EmptyArray`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1228516547
##########
src/commands/cmd_zset.cc:
##########
@@ -555,9 +561,17 @@ class CommandZRangeGeneric : public Commander {
s = zset_db.RangeByRank(key_, rank_spec_, &member_scores, nullptr);
break;
case kZRangeScore:
+ if (score_spec_.count == 0) {
+ *output = redis::EmptyArray();
+ return Status::OK();
+ }
s = zset_db.RangeByScore(key_, score_spec_, &member_scores, nullptr);
break;
case kZRangeLex:
+ if (lex_spec_.count == 0) {
+ *output = redis::EmptyArray();
Review Comment:
we can use `std::vector<std::string> values; *output =MultiBulkString(values, false)` or `*output=redis::MultiLen(0)` to do the same thing.
I should remove the specific `EmptyArray`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#discussion_r1227985464
##########
src/commands/cmd_zset.cc:
##########
@@ -513,6 +520,8 @@ class CommandZRangeGeneric : public Commander {
max_idx = 2;
}
+ if (count == 0) zero_count_ = true;
Review Comment:
https://github.com/apache/incubator-kvrocks/blob/e89af5289d5c3d12c47628394c72f0d67e652800/src/commands/cmd_zset.cc#L552-L566
Ok, I will add three statements in here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1492: Fix zset error: return nilstring when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1586192161
=== RUN TestZset/ZRANGE_basics_-_skiplist
zset_test.go:445:
Error Trace: ~/incubator-kvrocks/tests/gocase/unit/type/zset/zset_test.go:445
Error: Not equal:
expected: []string{}
actual : []string(nil)
Diff:
--- Expected
+++ Actual
@@ -1,3 +1,2 @@
-([]string) {
-}
+([]string) <nil>
Test: TestZset/ZRANGE_basics_-_skiplist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1492: Fix zset error: return the empty array when limit cnt == 0
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1492:
URL: https://github.com/apache/incubator-kvrocks/pull/1492#issuecomment-1587703348
> @infdahai You can use expression like `require.EqualError(t, r.Err(), redis.Nil.Error())`
Yeah. I find the correct msg is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org