Add SINTER, SUNION, SDIFF, SPOP and SRANDMEMBER set commands#50
Add SINTER, SUNION, SDIFF, SPOP and SRANDMEMBER set commands#50tchalikanti1705 wants to merge 10 commits into
Conversation
Rohit-Dnath
left a comment
There was a problem hiding this comment.
Close, and the hard parts are right. One real bug to fix before I merge.
First, the things I specifically tried to break and could not. readSets takes one shard RLock at a time and releases it before the next, so there is no lock-ordering hazard and no same-shard double-RLock, which is the subtle one I was most worried about (RLock twice from one goroutine deadlocks if a writer arrives in between). SINTER a a works. SPOP's bulk-vs-array reply difference between the no-count and count forms is right, the key is deleted when emptied, and SRANDMEMBER's negative count allowing duplicates is right, which is the one people usually get wrong. Expiry uses getLive on the write path and peekLive on the reads. Good work.
The bug: SINTER skips type-checking keys after the first missing one
readSets(keys, stopOnMissing=true) returns on the first missing key, so later keys never get type-checked. The comment says this mirrors Redis. It does not. Redis 7.4's sinterGenericCommand continues past a NULL key, type-checks every key, and only then returns empty:
if (!setobj) { empty += 1; sets[j] = NULL; continue; }
if (checkType(c,setobj,OBJ_SET)) { zfree(sets); return; }With SADD a 1 and SET str v:
| command | this PR | Redis 7.4 |
|---|---|---|
SINTER a nope str |
[] |
WRONGTYPE |
SINTER a str nope |
WRONGTYPE | WRONGTYPE |
SDIFF a nope str |
WRONGTYPE | WRONGTYPE |
Two things bother me here beyond the Redis mismatch. The result depends on argument order, so the same three keys give an error or an empty array based only on how they were written, which reads as nondeterministic from a client. And SINTER now disagrees with SDIFF and SUNION on identical inputs, which is the kind of inconsistency that costs someone an afternoon later.
The fix drops stopOnMissing entirely: always type-check every key, track whether any was missing, and let SInter return empty if one was. That also collapses readSets back to a single mode and removes the complete return value, so it is less code than what is there now.
Your tests currently assert the wrong behaviour (SInter missing-before-wrongtype in store_test.go, plus the server_test.go case), so those need updating with it. Flagging that explicitly because a passing suite would otherwise make this look fine.
Not blocking
SRANDMEMBER key -Ncaps at 2^20 where Redis has no cap. Defensible as a DoS guard and you documented it in the code, butdocs/commands.mddoes not mention it. Worth a line there.- The multi-key algebra is not atomic, since each shard lock drops before the next. Redis does these atomically. This is the right trade against multi-shard locking and your comment cites the MGET convention, so I am fine with it. Noting it as a known limitation, not a change request.
SINTER now stops at the first missing key and returns an empty result without type-checking the keys after it, so SINTER a missing wrongtype is empty rather than WRONGTYPE. SRANDMEMBER applies its with-repetition cap only after confirming the key exists and holds a set, so a missing key is still empty and a wrong type still WRONGTYPEs even for a huge count.
SINTER used to stop at the first missing key and skip the keys after it, so SINTER a nope str returned empty while SINTER a str nope was WRONGTYPE. Redis type-checks every key before deciding the result, so the reply no longer depends on argument order and SINTER agrees with SUNION and SDIFF on the same inputs. This drops readSets' stopOnMissing mode entirely; a missing key is just an empty set.
54b4e29 to
185ac5c
Compare
|
Fixed as you described — dropped stopOnMissing entirely, so every key is type-checked and SINTER agrees with SUNION/SDIFF regardless of argument order. Updated the tests that were asserting the old behavior and added both argument orders over the wire, plus the docs line for the SRANDMEMBER cap. Also rebased onto main. |
Summary
Adds the set commands
SINTER,SUNION,SDIFF,SPOP, andSRANDMEMBERso RAMen's sets support the multi-set operations and random/pop helpers, alongside the existingSADD/SREM/SMEMBERS/SISMEMBER/SCARD.Related Issue
Closes #9
Changes
SINTER <key> [key ...]/SUNION <key> [key ...]/SDIFF <key> [key ...]— set intersection, union, and difference; a missing key is treated as an empty set, and any key holding a non-set returns WRONGTYPESPOP <key> [count]— remove and return one random member as a bulk string (nil if missing), or an array of up tocountmembers; the key is deleted when emptied, and a negative count returnsERR value is out of range, must be positiveSRANDMEMBER <key> [count]— return random member(s) without modifying the set: no count returns one (bulk, nil if missing), a positive count up to that many distinct members, a negative count exactly-countmembers with repetitionSInter,SUnion,SDiff(sharing areadSetshelper) plusSPopandSRandMemberininternal/store/set.go; SRANDMEMBER's with-repetition count is bounded to avoid an unbounded allocation from a client-supplied magnitudeinternal/server/cmd_set.goandcommands.gointernal/store/store_test.goandinternal/server/server_test.go, and documented the commands indocs/commands.mdChecklist
go test ./...)go fmt ./...)