SPOP optional count argument. (issue #1793, supersedes pull request #1803) #2215

Merged
merged 1 commit into from Dec 17, 2014

Conversation

Projects
None yet
3 participants
@advance512
Contributor

advance512 commented Dec 14, 2014

More details of implementation in issue #1793.
Previous pull request #1803, based on antirez:3.0 is now deprecated. This pull request supersedes it.


Commit message:

Added parameter to SPOP:

  • spopCommand() now runs spopWithCountCommand() in case the param is found.
  • Added intsetRandomMembers() to Intset: Copies N random members from the set into inputted 'values' array. Uses either the Knuth or Floyd sample algos depending on ratio count/size.
  • Added setTypeRandomElements() to SET type: Returns a number of random elements from a non empty set. This is a version of setTypeRandomElement() that is modified in order to return multiple entries, using dictGetRandomKeys() and intsetRandomMembers().
  • Added tests for SPOP with : unit/type/set, unit/scripting, integration/aof

Cleaned up code a bit to match with required Redis coding style

Added <count> parameter to SPOP:
spopCommand() now runs spopWithCountCommand() in case the <count> param is found.
Added intsetRandomMembers() to Intset: Copies N random members from the set into inputted 'values' array. Uses either the Knuth or Floyd sample algos depending on ratio count/size.
Added setTypeRandomElements() to SET type: Returns a number of random elements from a non empty set. This is a version of setTypeRandomElement() that is modified in order to return multiple entries, using dictGetRandomKeys() and intsetRandomMembers().
Added tests for SPOP with <count>: unit/type/set, unit/scripting, integration/aof
--
Cleaned up code a bit to match with required Redis coding style

antirez added a commit that referenced this pull request Dec 17, 2014

Merge pull request #2215 from advance512/spopWithCount
SPOP optional count argument. (issue #1793, supersedes pull request #1803)

@antirez antirez merged commit 70674ac into antirez:unstable Dec 17, 2014

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Dec 17, 2014

Owner

Merged, congrats for the high quality of the contribution. Thank you.

Owner

antirez commented Dec 17, 2014

Merged, congrats for the high quality of the contribution. Thank you.

@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Dec 18, 2014

Contributor

This introduced a warning:

t_set.c:548:35: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
    redisAssert(elements_returned == count);
                ~~~~~~~~~~~~~~~~~ ^  ~~~~~
./redis.h:402:27: note: expanded from macro 'redisAssert'
#define redisAssert(_e) ((_e)?(void)0 : (_redisAssert(#_e,__FILE__,__LINE__),_exit(1)))
Contributor

mattsta commented Dec 18, 2014

This introduced a warning:

t_set.c:548:35: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
    redisAssert(elements_returned == count);
                ~~~~~~~~~~~~~~~~~ ^  ~~~~~
./redis.h:402:27: note: expanded from macro 'redisAssert'
#define redisAssert(_e) ((_e)?(void)0 : (_redisAssert(#_e,__FILE__,__LINE__),_exit(1)))
@advance512

This comment has been minimized.

Show comment
Hide comment
@advance512

advance512 Dec 18, 2014

Contributor

Sorry, I didn't get the warning, since the default Makefile uses gcc with the flag -Wall and the warning only appears using -Wextra (which enables -Wsign-compare). Maybe the default warning flag should change?

The warning actually exposed an interesting issue: I used int instead of unsigned long in setTypeRandomElements(), and so in the extreme case of SPOP being called with COUNT>MAXINT, an unsigned->signed conversion could have caused an issue (negative COUNT argument).

I doubt there's a use case which would ever need such a large COUNT, but still - it is an issue.

I created a fix, found in Pull Request #2224.

Contributor

advance512 commented Dec 18, 2014

Sorry, I didn't get the warning, since the default Makefile uses gcc with the flag -Wall and the warning only appears using -Wextra (which enables -Wsign-compare). Maybe the default warning flag should change?

The warning actually exposed an interesting issue: I used int instead of unsigned long in setTypeRandomElements(), and so in the extreme case of SPOP being called with COUNT>MAXINT, an unsigned->signed conversion could have caused an issue (negative COUNT argument).

I doubt there's a use case which would ever need such a large COUNT, but still - it is an issue.

I created a fix, found in Pull Request #2224.

@advance512

This comment has been minimized.

Show comment
Hide comment
@advance512

advance512 Dec 18, 2014

Contributor

Also, thank you for the kind words @antirez :)

Contributor

advance512 commented Dec 18, 2014

Also, thank you for the kind words @antirez :)

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Dec 18, 2014

Owner

Great, fix merged, thank you both.

Owner

antirez commented Dec 18, 2014

Great, fix merged, thank you both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment