You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "raghav2005 (via GitHub)" <gi...@apache.org> on 2026/08/30 14:35:13 UTC
[PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
raghav2005 opened a new pull request, #3601:
URL: https://github.com/apache/kvrocks/pull/3601
## Summary
- return a Redis-compatible bulk string for `SRANDMEMBER key`
- return nil for a missing key when count is omitted
- preserve collection replies for `SRANDMEMBER key count`
- add regression coverage for scalar, counted, and missing-key reply types
Closes #2610.
## Testing
- `./x.py build build-docker --unittest --ninja -j 4`
- `./x.py check format`
- `./x.py check tidy build-docker -j 4`
- `./x.py check golangci-lint`
- `./x.py test go build-docker`
- `/workspace/build-docker/unittest` (623 passed, 1 skipped)
- `/workspace/build-docker/unittest --gtest_filter=RedisSetTest.TakeWithoutPop`
## AI assistance
I used OpenAI Codex to help inspect the issue and related command patterns, draft the focused implementation and regression test, and run and review the repository checks. I reviewed and understand the resulting change and take responsibility for it.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#discussion_r3906765948
##########
tests/gocase/unit/protocol/protocol_test.go:
##########
@@ -298,6 +298,20 @@ func TestProtocolRESP3(t *testing.T) {
}
})
+ t.Run("SRANDMEMBER with count returns an array", func(t *testing.T) {
+ require.NoError(t, c.WriteArgs("SADD", "srandmember-resp3", "member"))
+ c.MustRead(t, ":1")
+
+ require.NoError(t, c.WriteArgs("SRANDMEMBER", "srandmember-resp3-missing", "1"))
+ c.MustRead(t, "*0")
+
+ require.NoError(t, c.WriteArgs("SRANDMEMBER", "srandmember-resp3", "-2"))
+ c.MustRead(t, "*2")
+ for range 2 {
+ c.MustReadBulkString(t, "member")
+ }
+ })
Review Comment:
This RESP3 protocol test validates the counted form (array) but doesn’t assert the raw RESP3 reply type for the scalar form (`SRANDMEMBER key` should be a bulk string) nor the missing-key scalar form (should be RESP3 null `_`). Since this PR changes the scalar return shape, adding these assertions would prevent regressions at the protocol layer.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#issuecomment-5503297060
Good catch.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "raghav2005 (via GitHub)" <gi...@apache.org>.
raghav2005 commented on code in PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#discussion_r3905948790
##########
src/commands/cmd_set.cc:
##########
@@ -219,12 +220,18 @@ class CommandSRandMember : public Commander {
if (!s.ok()) {
return {Status::RedisExecErr, s.ToString()};
}
- *output = conn->SetOfBulkStrings(members);
+
+ if (with_count_) {
+ *output = conn->SetOfBulkStrings(members);
+ } else {
+ *output = members.empty() ? conn->NilString() : redis::BulkString(members.front());
+ }
Review Comment:
Fixed in 3fd3ad6d. The counted form now uses redis::ArrayOfBulkStrings, so RESP3 emits an array instead of a set. I added raw RESP3 coverage for an empty counted result and for a negative count that returns duplicates. The full Go and C++ suites, clang-tidy, formatting, and Go lint all pass.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#issuecomment-5469300701
Hi @raghav2005,
Thank you for your pull request. Please review our [Contributing Guide](https://kvrocks.apache.org/community/contributing/).
Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#discussion_r3899673129
##########
src/commands/cmd_set.cc:
##########
@@ -219,12 +220,18 @@ class CommandSRandMember : public Commander {
if (!s.ok()) {
return {Status::RedisExecErr, s.ToString()};
}
- *output = conn->SetOfBulkStrings(members);
+
+ if (with_count_) {
+ *output = conn->SetOfBulkStrings(members);
+ } else {
+ *output = members.empty() ? conn->NilString() : redis::BulkString(members.front());
+ }
Review Comment:
For `SRANDMEMBER key count`, the reply should be an array (even under RESP3). Using `conn->SetOfBulkStrings` emits a RESP3 Set (`~`), which is not Redis-compatible for SRANDMEMBER, and is also semantically wrong when `count` is negative (duplicates are allowed but a set reply implies uniqueness). Prefer emitting an array of bulk strings for the counted form.
--
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
Re: [PR] fix(set): return scalar for SRANDMEMBER without count [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #3601:
URL: https://github.com/apache/kvrocks/pull/3601#discussion_r3910107952
##########
tests/gocase/unit/protocol/protocol_test.go:
##########
@@ -298,6 +298,20 @@ func TestProtocolRESP3(t *testing.T) {
}
})
+ t.Run("SRANDMEMBER with count returns an array", func(t *testing.T) {
+ require.NoError(t, c.WriteArgs("SADD", "srandmember-resp3", "member"))
+ c.MustRead(t, ":1")
+
+ require.NoError(t, c.WriteArgs("SRANDMEMBER", "srandmember-resp3-missing", "1"))
+ c.MustRead(t, "*0")
+
+ require.NoError(t, c.WriteArgs("SRANDMEMBER", "srandmember-resp3", "-2"))
+ c.MustRead(t, "*2")
+ for range 2 {
+ c.MustReadBulkString(t, "member")
+ }
+ })
Review Comment:
@raghav2005 I feel this suggestion did make sense.
--
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